Received-SPF: pass (google.com: domain of linux-kernel+bounces-78279-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0fa391c6-fd9e-42fe-b535-17e7725280e5@arm.com>
Date: Fri, 23 Feb 2024 17:40:02 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/3] perf tools: Only treat files as map files when they
 have the extension .map
To: Arnaldo Carvalho de Melo <acme@kernel.org>,
 Ian Rogers <irogers@google.com>
Cc: linux-perf-users@vger.kernel.org, anshuman.khandual@arm.com,
 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20240220042957.2022391-1-ChaitanyaS.Prakash@arm.com>
 <20240220042957.2022391-3-ChaitanyaS.Prakash@arm.com>
 <CAP-5=fUFmeoTjLuZTgcaV23iGQU1AdddG+7Rw=d6buMU007+1Q@mail.gmail.com>
 <ZdYPf3wMl35VemsL@x1>
Content-Language: en-US
From: Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com>
In-Reply-To: <ZdYPf3wMl35VemsL@x1>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

I'll make the changes, thanks for the review.

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@arm.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@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