Re: [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map

[Chaitanya S Prakash]

On 3/16/24 02:04, Arnaldo Carvalho de Melo wrote:
> On Fri, Feb 23, 2024 at 05:40:02PM +0530, Chaitanya S Prakash wrote:
> > I'll make the changes, thanks for the review.
> Have you submitted a new series?
>
> Thanks,
>
> - Arnaldo
I will be posting it this week, extremely sorry for the delay!
>   
> > On 2/21/24 20:28, Arnaldo Carvalho de Melo wrote:
> > > On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
> > > > On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@xxxxxxx> wrote:
> > > > > +++ b/tools/perf/util/string2.h
> > > > > @@ -40,5 +40,6 @@ char *strdup_esc(const char *str);
> > > > >
> > > > >    unsigned int hex(char c);
> > > > >    char *strreplace_chars(char needle, const char *haystack, const char *replace);
> > > > > +const char *ends_with(const char *str, const char *suffix);
> > > > nit: string2.h is an extension of linux's string.h. The tools copy of
> > > > that is missing functions in the kernel version:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/include/linux/string.h?h=perf-tools-next
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/string.h?h=perf-tools-next#n398
> > > > specifically str_has_prefix.
> > > >
> > > > The naming ends_with makes sense but there is also strstarts and
> > > > str_has_prefix, perhaps str_has_suffix would be the most consistent
> > > > and intention revealing name?
> > > >
> > > > Also, we have strtailcmp which behaves like a reverse strcmp that
> > > > doesn't compare the lengths of the strings. It seems all uses of
> > > > strtailcmp are just for a "str_has_suffix". It would make sense to me
> > > > to remove that function and switch to a common str_has_suffix function
> > > > which I think is a more intention revealing way of naming what the
> > > > code is doing.
> > > So far in perf we try to just reuse whatever function the kernel has for
> > > the purpose at hand, right now the kernel has:
> > >
> > > /**
> > >    * strstarts - does @str start with @prefix?
> > >    * @str: string to examine
> > >    * @prefix: prefix to look for.
> > >    */
> > > static inline bool strstarts(const char *str, const char *prefix)
> > > {
> > >           return strncmp(str, prefix, strlen(prefix)) == 0;
> > > }
> > >
> > > And:
> > >
> > > /**
> > >    * str_has_prefix - Test if a string has a given prefix
> > >    * @str: The string to test
> > >    * @prefix: The string to see if @str starts with
> > >    *
> > >    * A common way to test a prefix of a string is to do:
> > >    *  strncmp(str, prefix, sizeof(prefix) - 1)
> > >    *
> > >    * But this can lead to bugs due to typos, or if prefix is a pointer
> > >    * and not a constant. Instead use str_has_prefix().
> > >    *
> > >    * Returns:
> > >    * * strlen(@prefix) if @str starts with @prefix
> > >    * * 0 if @str does not start with @prefix
> > >    */
> > > static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> > > {
> > >           size_t len = strlen(prefix);
> > >           return strncmp(str, prefix, len) == 0 ? len : 0;
> > > }
> > >
> > > The later seems to give more bang for the buck, so to say, returning the
> > > prefix lenght.
> > >
> > > It is a new addition:
> > >
> > > 72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> > >
> > > While:
> > >
> > > 66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?
> > >
> > > ⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
> > > 94
> > > ⬢[acme@toolbox linux]$ git grep strstarts| wc -l
> > > 177
> > > ⬢[acme@toolbox linux]$
> > >
> > > Some places use it:
> > >
> > > kernel/printk/printk.c: len = str_has_prefix(str, "on");
> > > kernel/printk/printk.c: len = str_has_prefix(str, "off");
> > > kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");
> > >
> > >
> > > static int __control_devkmsg(char *str)
> > > {
> > > 	size_t len;
> > >
> > > 	if (!str)
> > > 		return -EINVAL;
> > >
> > > 	len = str_has_prefix(str, "on");
> > > 	if (len) {
> > > 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
> > > 		return len;
> > > 	}
> > >
> > > 	len = str_has_prefix(str, "off");
> > > 	if (len) {
> > > 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
> > > 		return len;
> > > 	}
> > >
> > > 	len = str_has_prefix(str, "ratelimit");
> > > 	if (len) {
> > > 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
> > > 		return len;
> > > 	}
> > >
> > > 	return -EINVAL;
> > > }
> > >
> > >
> > >                   err = __control_devkmsg(devkmsg_log_str);
> > >                   /*
> > >                    * Do not accept an unknown string OR a known string with
> > >                    * trailing crap...
> > >                    */
> > >                   if (err < 0 || (err + 1 != *lenp)) {
> > >
> > >                           /* ... and restore old setting. */
> > >                           devkmsg_log = old;
> > >                           strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
> > >
> > >                           return -EINVAL;
> > >                   }
> > >
> > >
> > > So yeah, I agree with Ian that it is more intention revealing, has this
> > > bonus of returning the strlen for the above use cases, is in the kernel
> > > sources, so I'm in favour of grabbing a copy of it and replacing the
> > > strstarts() usage with it, drop strstarts(), then also introduce
> > > str_has_suffix(), the kernel will get it when it needs, possibly from
> > > tools/lib/ :-)
> > >
> > > - Arnaldo