Date: Wed, 21 Feb 2024 11:58:07 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Chaitanya S Prakash <ChaitanyaS.Prakash@....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 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