| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ad9bdae3-0eb2-4262-8412-ba42c27ee767@arm.com>
Date: Tue, 19 Mar 2024 07:56:02 +0530
From: Chaitanya S Prakash <ChaitanyaS.Prakash@....com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
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 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@....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