Received-SPF: pass (google.com: domain of linux-kernel+bounces-106919-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <ad9bdae3-0eb2-4262-8412-ba42c27ee767@arm.com>
Date: Tue, 19 Mar 2024 07:56: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>
Cc: Ian Rogers <irogers@google.com>, 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> <0fa391c6-fd9e-42fe-b535-17e7725280e5@arm.com>
 <ZfSw7AL8EgFoFQkM@x1>
Content-Language: en-US
From: Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com>
In-Reply-To: <ZfSw7AL8EgFoFQkM@x1>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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