Date: Fri, 15 Mar 2024 17:34:52 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Chaitanya S Prakash <ChaitanyaS.Prakash@....com>
Cc: Ian Rogers <irogers@...gle.com>, linux-perf-users@...r.kernel.org,
	anshuman.khandual@....com,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] perf tools: Only treat files as map files when they
 have the extension .map

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
 
> 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@....com> 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@...lbox linux]$ git grep str_has_prefix| wc -l
> > 94
> > ⬢[acme@...lbox linux]$ git grep strstarts| wc -l
> > 177
> > ⬢[acme@...lbox 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

Powered by blists - more mailing lists