Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Mon, 24 Dec 2018 00:52:13 +0100 Rasmus Villemoes wrote: > On 23/12/2018 23.56, Steven Rostedt wrote: > > On Sun, 23 Dec 2018 23:01:52 +0100 > > Rasmus Villemoes wrote: > > > >> On 21/12/2018 23.20, Joe Perches wrote: > >>> > >>> Using > >>> > >>> static inline bool str_has_prefix(const char *str, const char prefix[]) > >>> { > >>> return !strncmp(str, prefix, strlen(prefix)); > >>> } > >>> > >> > >> We already have exactly that function, it's called strstarts(). > > > > It's not exact. > > Huh? The str_has_prefix() I quoted is exactly strstarts(). The the implemented str_has_prefix() that you replied to is: +/* + * 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 strncmp_prefix(). + */ +#define strncmp_prefix(str, prefix)\ + ({ \ + int strcmp_prefix_ret; \ + if (__builtin_constant_p()) {\ + strcmp_prefix_ret = \ + strncmp(str, prefix, sizeof(prefix) - 1); \ + } else {\ + typeof(prefix) strcmp_prefix = prefix; \ + strcmp_prefix_ret = \ + strncmp(str, strcmp_prefix, \ + strlen(strcmp_prefix)); \ + } \ + strcmp_prefix_ret; \ + }) + Note, this has turned into a nice function: http://lkml.kernel.org/r/20181222162856.518489...@goodmis.org +/** + * 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: 0 if @str does not start with @prefix + strlen(@prefix) if @str does 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; +} + > > > > > Well, one thing that str_has_prefix() does that strstarts() does not, > > is to return the prefix length which gets rid of the duplication. > > I hadn't seen the patches containing that version of str_has_prefix(). > Anyway, I just wanted to point out that strstarts() exists, so that we > at least do not add a copy of that. That's because you didn't read the patch that you quoted, just the change log. > > > Would it be OK to convert strstarts() to return the length of prefix? > > Dunno. By far, most users of the strncmp() idiom only seem to be > interested in the boolean result. It's true that there are some that > then want to skip the prefix and do further parsing, and I can see how > avoiding duplicating the prefix length is useful. But the mathematician > in me can't help consider the corner case of 'the empty string is always > a prefix of any other string', and having str_has_prefix(str, "") be > false seems wrong - obviously, nobody would ever use a literal "" there, > but nothing in str_has_prefix() _requires_ the prefix to be a constant. Which would be a useless use case. And if you define that it returns the length of prefix on return, then it both matches and doesn't match ;-) > > Maybe 'bool str_skip_prefix(const char *s, const char *p, const char > **out)' where *out is set to s+len on success, and set to s on failure > (just to allow passing and continue parsing in elseifs)? That would > make your 4/5 "tracing: Have the historgram use the result of > str_has_prefix() for len of prefix" do > > if (str_skip_prefix(str, "onmatch(", _str)) { > > hoisting the action_str declaration to the top, replacing the len variable? > The use cases I've used in the final patch series uses the len for indexing and other cases. I think I'm keeping the str_has_prefix() and change the other users to use it in the kernel. Most of the git grep strstarts() is tools and scripts anyway. -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On 23/12/2018 23.56, Steven Rostedt wrote: > On Sun, 23 Dec 2018 23:01:52 +0100 > Rasmus Villemoes wrote: > >> On 21/12/2018 23.20, Joe Perches wrote: >>> >>> Using >>> >>> static inline bool str_has_prefix(const char *str, const char prefix[]) >>> { >>> return !strncmp(str, prefix, strlen(prefix)); >>> } >>> >> >> We already have exactly that function, it's called strstarts(). > > It's not exact. Huh? The str_has_prefix() I quoted is exactly strstarts(). > > Well, one thing that str_has_prefix() does that strstarts() does not, > is to return the prefix length which gets rid of the duplication. I hadn't seen the patches containing that version of str_has_prefix(). Anyway, I just wanted to point out that strstarts() exists, so that we at least do not add a copy of that. > Would it be OK to convert strstarts() to return the length of prefix? Dunno. By far, most users of the strncmp() idiom only seem to be interested in the boolean result. It's true that there are some that then want to skip the prefix and do further parsing, and I can see how avoiding duplicating the prefix length is useful. But the mathematician in me can't help consider the corner case of 'the empty string is always a prefix of any other string', and having str_has_prefix(str, "") be false seems wrong - obviously, nobody would ever use a literal "" there, but nothing in str_has_prefix() _requires_ the prefix to be a constant. Maybe 'bool str_skip_prefix(const char *s, const char *p, const char **out)' where *out is set to s+len on success, and set to s on failure (just to allow passing and continue parsing in elseifs)? That would make your 4/5 "tracing: Have the historgram use the result of str_has_prefix() for len of prefix" do if (str_skip_prefix(str, "onmatch(", _str)) { hoisting the action_str declaration to the top, replacing the len variable? Rasmus
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Sun, 2018-12-23 at 23:01 +0100, Rasmus Villemoes wrote: > On 21/12/2018 23.20, Joe Perches wrote: [] > > static inline bool str_has_prefix(const char *str, const char prefix[]) [] > We already have exactly that function, it's called strstarts(). Heh. Thanks Rasmus. I didn't remember that one. I think the 'int str_has_prefix' naming and returning the prefix length may be a bit better use than 'bool strstarts' and perhaps a treewide conversion of the existing strstarts to str_has_prefix would be OK as there aren't that many. $ git grep -w strstarts | wc -l 91
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Sun, 23 Dec 2018 23:01:52 +0100 Rasmus Villemoes wrote: > On 21/12/2018 23.20, Joe Perches wrote: > > On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote: > >> On Fri, 21 Dec 2018 21:58:32 +0100 > >> Andreas Schwab wrote: > >> > >> > Well, perhaps I can just remove the ending ones. I get paranoid with > macro variables, and tend to over do it so that there's no question. > >>> > >>> Why not make it an inline function? > >> > >> Matters if that removes the strlen(const) optimization. I could try it > >> and see what happens. > > > > Using > > > > static inline bool str_has_prefix(const char *str, const char prefix[]) > > { > > return !strncmp(str, prefix, strlen(prefix)); > > } > > > > We already have exactly that function, it's called strstarts(). It's not exact. > > commit 66f92cf9d415e96a5bdd6c64de8dd8418595d2fc > Author: Rusty Russell > Date: Tue Mar 31 13:05:36 2009 -0600 > > strstarts: helper function for !strncmp(str, prefix, strlen(prefix)) > > Please don't add a copy under another name. > > As for converting existing users, go for it. FWIW, I ran a cocci script > a few years ago to find suspicious strncmp() cases, and there were some > (e87c3f, ca957b6), but fewer than I expected. There are some > confused/confusing ones that apparently deliberately do strncmp(a, b, > sizeof(b)) instead of the equivalent to strcmp(a, b) (e.g. 'strncmp(str, > "hwc", 4) == 0') Well, one thing that str_has_prefix() does that strstarts() does not, is to return the prefix length which gets rid of the duplication. Would it be OK to convert strstarts() to return the length of prefix? -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On 21/12/2018 23.20, Joe Perches wrote: > On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote: >> On Fri, 21 Dec 2018 21:58:32 +0100 >> Andreas Schwab wrote: >> >> Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question. >>> >>> Why not make it an inline function? >> >> Matters if that removes the strlen(const) optimization. I could try it >> and see what happens. > > Using > > static inline bool str_has_prefix(const char *str, const char prefix[]) > { > return !strncmp(str, prefix, strlen(prefix)); > } > We already have exactly that function, it's called strstarts(). commit 66f92cf9d415e96a5bdd6c64de8dd8418595d2fc Author: Rusty Russell Date: Tue Mar 31 13:05:36 2009 -0600 strstarts: helper function for !strncmp(str, prefix, strlen(prefix)) Please don't add a copy under another name. As for converting existing users, go for it. FWIW, I ran a cocci script a few years ago to find suspicious strncmp() cases, and there were some (e87c3f, ca957b6), but fewer than I expected. There are some confused/confusing ones that apparently deliberately do strncmp(a, b, sizeof(b)) instead of the equivalent to strcmp(a, b) (e.g. 'strncmp(str, "hwc", 4) == 0') Rasmus
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Sat, 22 Dec 2018 10:20:02 + Malcolm Priestley wrote: > > Calling it "strncmp_prefix()" when there is no "n" there makes no sense. > > > > So drop the "n" from the name. > > > Being British the 'n' implies 'and' and still being interpreted Boolean. > > strncmp = string and compare > > Like other of our words Fish'n'Chips, Salt'n'Shake. > > I don't think these abbreviations exist in American English. No, no, they actually do exist in American English. But when people ask me which language is my mother tongue, I simply reply "C". In which case, 'n' means number. -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On 21/12/2018 18:51, Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt wrote: >> >> I figured the best thing to do is to create a helper macro and place it >> into include/linux/string.h. And go around and fix all the open coded >> versions of it later. >> >> I plan on only applying this patch and updating the tracing hooks for >> this merge window. And perhaps use it to fix some of the bugs that were >> found. > > I like the helper function concept, but as they say about CS: "There > is only one hard problem in computer science: naming and off-by-one > errors". > > And this one has that problem. The name makes absolutely no sense. > > Calling it "strncmp_prefix()" when there is no "n" there makes no sense. > > So drop the "n" from the name. > Being British the 'n' implies 'and' and still being interpreted Boolean. strncmp = string and compare Like other of our words Fish'n'Chips, Salt'n'Shake. I don't think these abbreviations exist in American English.
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 14:57:13 -0800 Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 2:48 PM Steven Rostedt wrote: > > > > > Your patch actually had them, but in the body of your email you did > > > > > > > #define have_prefix(str, prefix) ({ \ > > > > const char *__pfx = (const char *)prefix; \ > > > > > > which is just completely wrong. > > > > > > Considering your _old_ patch had the exact same bug, I really think > > > you need to start internalizing the whole "macro arguments *have* to > > > be properly protected" thing. > > > > Well, there's less with assignments that can go wrong than with other > > code. That is, there's little that can happen with "int x = arg;" where > > arg is the macro paramater to cause something really nasty. > > What's wrong, Steven? We are miscommunicating here... I was talking about the missing parenthesis on the original patch, which you stated was missing as well. And the original patch didn't have the typecast. > > The assignment is entirely irrelevant. > > The problem is the cast. > > A type cast has a very high priority, and so if you do > > (const char *)prefix > > it breaks completely if you might have something like"a+6" as the argument. > > Think about what if "a" is of type "unsigned long", for example? Yes, when writing the real code, I noticed that the typecast could cause issues without the parenthesis, and added them. The email you are replying to was saying there's not much that can go wrong with: #define MACRO(x) { \ int __p = x; \ I definitely can see something wrong with: #define MACRO(x) { \ int __p = (int)x; \ because exactly what you stated. There's nothing wrong with adding (x) for the first one, but it's unlikely anything will cause it harm. The second one the (x) *is* most definitely required. This is a long winded "I agree with you" ;-) -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 14:29:30 -0800 Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 2:20 PM Joe Perches wrote: > > > > Using > > > > static inline bool str_has_prefix(const char *str, const char prefix[]) > > { > > return !strncmp(str, prefix, strlen(prefix)); > > } > > > > eliminates the strlen with gcc 4.8 (oldest I still have) > > Ok, that looks like the right thing to do. > Agreed, and I posted a new version. I can start running it through my test suit (I'll update all the instances in the tracing directory to use it), and then it will be ready for a pull request by next week. I'll revert the top two patches from my for-next tree now. -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, Dec 21, 2018 at 2:48 PM Steven Rostedt wrote: > > > Your patch actually had them, but in the body of your email you did > > > > > #define have_prefix(str, prefix) ({ \ > > > const char *__pfx = (const char *)prefix; \ > > > > which is just completely wrong. > > > > Considering your _old_ patch had the exact same bug, I really think > > you need to start internalizing the whole "macro arguments *have* to > > be properly protected" thing. > > Well, there's less with assignments that can go wrong than with other > code. That is, there's little that can happen with "int x = arg;" where > arg is the macro paramater to cause something really nasty. What's wrong, Steven? The assignment is entirely irrelevant. The problem is the cast. A type cast has a very high priority, and so if you do (const char *)prefix it breaks completely if you might have something like"a+6" as the argument. Think about what if "a" is of type "unsigned long", for example? Linus
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 14:20:25 -0800 Joe Perches wrote: > static inline bool str_has_prefix(const char *str, const char prefix[]) > { > return !strncmp(str, prefix, strlen(prefix)); > } > > eliminates the strlen with gcc 4.8 (oldest I still have) I tested it a bit more before posting. I tested it against: gcc 4.5.1, 4.5.4, 4.6.3, 6.2.0, 7.2.0 and 8.2 And the strlen was eliminated each time. So this looks like the right approach :-) -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 14:08:11 -0800 Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt wrote: > > > > On Fri, 21 Dec 2018 12:41:17 -0800 > > Linus Torvalds wrote: > > > > > Parentheses > > > > ? > > Your patch actually had them, but in the body of your email you did > > > #define have_prefix(str, prefix) ({ \ > > const char *__pfx = (const char *)prefix; \ > > which is just completely wrong. > > Considering your _old_ patch had the exact same bug, I really think > you need to start internalizing the whole "macro arguments *have* to > be properly protected" thing. Well, there's less with assignments that can go wrong than with other code. That is, there's little that can happen with "int x = arg;" where arg is the macro paramater to cause something really nasty. The chances of that happening is up there with using only two underscores causing an issue. But they don't hurt to add. > > And I agree with Joe that using a million underscores just makes code > less legible. Two underscores at the beginning is the standard > namespace protection. Underscores at the end do nothing. And using > *more* than two is just crazy. The two are standard for static variables in C code, but that makes me worry about macros. I usually do at least three for macros. The underscores at the end, to me, make it more balanced and easier to read: strncmp(str, ___prefix___, ___len___); To me looks better than: strncmp(str, ___prefix, ___len); But again, no reason to fight this bikeshed. > > Finally, I think the cast is actually bogus and wrong. Why would the > prefix ever be anything but "const char *"? Adding the cast only makes > it more possible that somebody uses a truly wrong type ("unsigned long > *" ?), and then the cast just silently hides it. Actually after I sent the email, I was thinking the same thing, that the original strncmp() would probably complain about that too, and we should keep it the same. Not sure why I even suggested that. Perhaps, I tested too many use cases :-/ > > If somebody uses "unsigned char *" for this, they'd get the exact same > warning if they were using strncmp and do this by hand. > > So why would that helper function act any differently? Particularly > when it then has the huge downside that it will also accept absolute > garbage? > > Anyway, that was a long and winding NAK for your patch. But after all that and said. I tested it as a static __always_inline, and guess what. It also optimizes out the strlen() for constants!!! Suggested-by: Andreas Schwab -- Steve diff --git a/include/linux/string.h b/include/linux/string.h index 27d0482e5e05..538469dfb458 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -456,4 +456,25 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len, memcpy(dest, src, dest_len); } +/** + * 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: 0 if @str does not start with @prefix + strlen(@prefix) if @str does start with @prefix + */ +static __always_inline int str_has_prefix(const char *str, const char *prefix) +{ + int len = strlen(prefix); + + return strncmp(str, prefix, len) == 0 ? len : 0; +} + #endif /* _LINUX_STRING_H_ */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, Dec 21, 2018 at 2:20 PM Joe Perches wrote: > > Using > > static inline bool str_has_prefix(const char *str, const char prefix[]) > { > return !strncmp(str, prefix, strlen(prefix)); > } > > eliminates the strlen with gcc 4.8 (oldest I still have) Ok, that looks like the right thing to do. Side note: in the kernel we disable the whole "unsigned vs signed" pointer warning entirely, exactly because of the "char *" vs "unsigned char *" problem. Linus
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote: > On Fri, 21 Dec 2018 21:58:32 +0100 > Andreas Schwab wrote: > > > > > Well, perhaps I can just remove the ending ones. I get paranoid with > > > macro variables, and tend to over do it so that there's no question. > > > > Why not make it an inline function? > > Matters if that removes the strlen(const) optimization. I could try it > and see what happens. Using static inline bool str_has_prefix(const char *str, const char prefix[]) { return !strncmp(str, prefix, strlen(prefix)); } eliminates the strlen with gcc 4.8 (oldest I still have) $ cat lib/test_module.c * This module emits "Hello, world" on printk when loaded. * * It is designed to be used for basic evaluation of the module loading * subsystem (for example when validating module signing/verification). It * lacks any extra dependencies, and will not normally be loaded by the * system unless explicitly requested by name. */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include #include #include static inline bool str_has_prefix(const char *str, const char prefix[]) { return !strncmp(str, prefix, strlen(prefix)); } bool test_str_has_prefix(const char *foo) { return str_has_prefix("TomJonesPrefix", foo); } bool test_str_has_prefix_TomJones(void) { return str_has_prefix("TomJonesPrefix", "TomJones"); } $ make CC=/usr/bin/gcc-4.8 lib/test_module.o $ objdump -d -s lib/test_module.o lib/test_module.o: file format elf64-x86-64 Contents of section .text: 534889fb e800 00b90f00 4883 SHH. 0010 f80f4889 df480f4e c848c7c6 ..H..H.N.H.. 0020 4839c9f3 a65b0f94 c0c3660f 1f44 H9...[f..D.. 0030 b801 00c3.. Contents of section .rodata.str1.1: 546f6d4a 6f6e6573 50726566 697800TomJonesPrefix. Contents of section .comment: 00474343 3a202855 62756e74 7520342e .GCC: (Ubuntu 4. 0010 382e352d 34756275 6e747539 2920342e 8.5-4ubuntu9) 4. 0020 382e3500 8.5. Contents of section .orc_unwind_ip: 0010 Contents of section .orc_unwind: 0800 05001000 0500 0800 0010 0500 0800 0500 0020 Disassembly of section .text: : 0: 53 push %rbx 1: 48 89 fbmov%rdi,%rbx 4: e8 00 00 00 00 callq 9 9: b9 0f 00 00 00 mov$0xf,%ecx e: 48 83 f8 0f cmp$0xf,%rax 12: 48 89 dfmov%rbx,%rdi 15: 48 0f 4e c8 cmovle %rax,%rcx 19: 48 c7 c6 00 00 00 00mov$0x0,%rsi 20: 48 39 c9cmp%rcx,%rcx 23: f3 a6 repz cmpsb %es:(%rdi),%ds:(%rsi) 25: 5b pop%rbx 26: 0f 94 c0sete %al 29: c3 retq 2a: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) 0030 : 30: b8 01 00 00 00 mov$0x1,%eax 35: c3 retq
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt wrote: > > On Fri, 21 Dec 2018 12:41:17 -0800 > Linus Torvalds wrote: > > > Parentheses > > ? Your patch actually had them, but in the body of your email you did > #define have_prefix(str, prefix) ({ \ > const char *__pfx = (const char *)prefix; \ which is just completely wrong. Considering your _old_ patch had the exact same bug, I really think you need to start internalizing the whole "macro arguments *have* to be properly protected" thing. And I agree with Joe that using a million underscores just makes code less legible. Two underscores at the beginning is the standard namespace protection. Underscores at the end do nothing. And using *more* than two is just crazy. Finally, I think the cast is actually bogus and wrong. Why would the prefix ever be anything but "const char *"? Adding the cast only makes it more possible that somebody uses a truly wrong type ("unsigned long *" ?), and then the cast just silently hides it. If somebody uses "unsigned char *" for this, they'd get the exact same warning if they were using strncmp and do this by hand. So why would that helper function act any differently? Particularly when it then has the huge downside that it will also accept absolute garbage? Anyway, that was a long and winding NAK for your patch. Linus
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 21:58:32 +0100 Andreas Schwab wrote: > > Well, perhaps I can just remove the ending ones. I get paranoid with > > macro variables, and tend to over do it so that there's no question. > > Why not make it an inline function? Matters if that removes the strlen(const) optimization. I could try it and see what happens. -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Dez 21 2018, Steven Rostedt wrote: > On Fri, 21 Dec 2018 12:46:47 -0800 > Joe Perches wrote: > >> > + * @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 has_prefix(). >> > + * >> > + * Returns: 0 if @str does not start with @prefix >> > + strlen(@prefix) if @str does start with @prefix >> > + */ >> > +#define has_prefix(str, prefix) >> > \ >> > + ({ \ >> > + const char *prefix = (const char *)(prefix);\ >> > + int len = strlen(prefix); \ >> > + strncmp(str, prefix, len) == 0 ?\ >> > + len : 0;\ >> > + }) >> >> I think all the underscores are unnecessary and confusing. >> > > Well, perhaps I can just remove the ending ones. I get paranoid with > macro variables, and tend to over do it so that there's no question. Why not make it an inline function? Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 12:46:47 -0800 Joe Perches wrote: > I think all the underscores are unnecessary and confusing. Here. I removed a beginning and ending underscore from each variable ;-) -- Steve diff --git a/include/linux/string.h b/include/linux/string.h index 27d0482e5e05..7f8871f7 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t); extern void *vmemdup_user(const void __user *, size_t); extern void *memdup_user_nul(const void __user *, size_t); +/** + * 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: 0 if @str does not start with @prefix + strlen(@prefix) if @str does start with @prefix + */ +#define str_has_prefix(str, prefix)\ + ({ \ + const char *___prefix___ = (const char *)(prefix); \ + int ___len___ = strlen(___prefix___); \ + strncmp(str, ___prefix___, ___len___) == 0 ?\ + ___len___ : 0; \ + }) + /* * Include machine specific inline routines */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 12:41:17 -0800 Linus Torvalds wrote: > Parentheses ? -- Steve > > On Fri, Dec 21, 2018, 12:35 Steven Rostedt > > On Fri, 21 Dec 2018 12:01:48 -0800 > > Linus Torvalds wrote: > > > > > On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt > > wrote: > > > > > > > > OK, what about if we just use strlen() and say that this macro is not > > > > safe for parameters with side effects. > > > > > > I think gcc follows simple assignments just fine, and does the > > > optimized strlen() for them too. > > > > > > So why not just do > > > > > >#define have_prefix(str,prefix) ({ \ > > > const char *__pfx = prefix; \ > > > size_t __pfxlen = strlen(__pfx); \ > > > strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); }) > > > > > > and be done with it safely? > > > > > > The above is ENTIRELY untested. > > > > > > > At first I thought this would have issues, but with a slight change... > > > > #define have_prefix(str, prefix) ({ \ > > const char *__pfx = (const char *)prefix; \ > > > > > > And the rest the same, it appears to work. > > > > Need the cast because if for some reason someone passed in something > > like "const unsigned char" then it wouldn't work. But that's just a nit. > > > > So something like this then? > > > > -- Steve > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > index 27d0482e5e05..4586fee60194 100644 > > --- a/include/linux/string.h > > +++ b/include/linux/string.h > > @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t); > > extern void *vmemdup_user(const void __user *, size_t); > > extern void *memdup_user_nul(const void __user *, size_t); > > > > +/** > > + * have_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 has_prefix(). > > + * > > + * Returns: 0 if @str does not start with @prefix > > + strlen(@prefix) if @str does start with @prefix > > + */ > > +#define has_prefix(str, prefix) > > \ > > + ({ \ > > + const char *prefix = (const char *)(prefix);\ > > + int len = strlen(prefix); \ > > + strncmp(str, prefix, len) == 0 ?\ > > + len : 0;\ > > + }) > > + > > /* > > * Include machine specific inline routines > > */ > >
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 12:46:47 -0800 Joe Perches wrote: > > @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t); > > extern void *vmemdup_user(const void __user *, size_t); > > extern void *memdup_user_nul(const void __user *, size_t); > > > > +/** > > + * have_prefix - Test if a string has a given prefix > > There is a naming mismatch of have/has here > and has_prefix is probably too generic a name. > > How about str_has_prefix? Sure. > > > + * @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 has_prefix(). > > + * > > + * Returns: 0 if @str does not start with @prefix > > + strlen(@prefix) if @str does start with @prefix > > + */ > > +#define has_prefix(str, prefix) > > \ > > + ({ \ > > + const char *prefix = (const char *)(prefix);\ > > + int len = strlen(prefix); \ > > + strncmp(str, prefix, len) == 0 ?\ > > + len : 0;\ > > + }) > > I think all the underscores are unnecessary and confusing. > Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question. I don't find them confusing ;-) But I tend to use a lot of macros. -- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 2018-12-21 at 15:35 -0500, Steven Rostedt wrote: > At first I thought this would have issues, but with a slight change... > > #define have_prefix(str, prefix) ({ \ > const char *__pfx = (const char *)prefix; \ > > > And the rest the same, it appears to work. > > Need the cast because if for some reason someone passed in something > like "const unsigned char" then it wouldn't work. But that's just a nit. > > So something like this then? > > -- Steve > > diff --git a/include/linux/string.h b/include/linux/string.h [] > @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t); > extern void *vmemdup_user(const void __user *, size_t); > extern void *memdup_user_nul(const void __user *, size_t); > > +/** > + * have_prefix - Test if a string has a given prefix There is a naming mismatch of have/has here and has_prefix is probably too generic a name. How about str_has_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 has_prefix(). > + * > + * Returns: 0 if @str does not start with @prefix > + strlen(@prefix) if @str does start with @prefix > + */ > +#define has_prefix(str, prefix) > \ > + ({ \ > + const char *prefix = (const char *)(prefix);\ > + int len = strlen(prefix); \ > + strncmp(str, prefix, len) == 0 ?\ > + len : 0;\ > + }) I think all the underscores are unnecessary and confusing.
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 12:01:48 -0800 Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt wrote: > > > > OK, what about if we just use strlen() and say that this macro is not > > safe for parameters with side effects. > > I think gcc follows simple assignments just fine, and does the > optimized strlen() for them too. > > So why not just do > >#define have_prefix(str,prefix) ({ \ > const char *__pfx = prefix; \ > size_t __pfxlen = strlen(__pfx); \ > strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); }) > > and be done with it safely? > > The above is ENTIRELY untested. > At first I thought this would have issues, but with a slight change... #define have_prefix(str, prefix) ({ \ const char *__pfx = (const char *)prefix; \ And the rest the same, it appears to work. Need the cast because if for some reason someone passed in something like "const unsigned char" then it wouldn't work. But that's just a nit. So something like this then? -- Steve diff --git a/include/linux/string.h b/include/linux/string.h index 27d0482e5e05..4586fee60194 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t); extern void *vmemdup_user(const void __user *, size_t); extern void *memdup_user_nul(const void __user *, size_t); +/** + * have_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 has_prefix(). + * + * Returns: 0 if @str does not start with @prefix + strlen(@prefix) if @str does start with @prefix + */ +#define has_prefix(str, prefix) \ + ({ \ + const char *prefix = (const char *)(prefix);\ + int len = strlen(prefix); \ + strncmp(str, prefix, len) == 0 ?\ + len : 0;\ + }) + /* * Include machine specific inline routines */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt wrote: > > OK, what about if we just use strlen() and say that this macro is not > safe for parameters with side effects. I think gcc follows simple assignments just fine, and does the optimized strlen() for them too. So why not just do #define have_prefix(str,prefix) ({ \ const char *__pfx = prefix; \ size_t __pfxlen = strlen(__pfx); \ strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); }) and be done with it safely? The above is ENTIRELY untested. Linus
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, 21 Dec 2018 10:51:29 -0800 Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt wrote: > > > > I figured the best thing to do is to create a helper macro and place it > > into include/linux/string.h. And go around and fix all the open coded > > versions of it later. > > > > I plan on only applying this patch and updating the tracing hooks for > > this merge window. And perhaps use it to fix some of the bugs that were > > found. > > I like the helper function concept, but as they say about CS: "There > is only one hard problem in computer science: naming and off-by-one > errors". > > And this one has that problem. The name makes absolutely no sense. Good thing I rebased and put these patches at the end before running my tests :-) (Specifically because I was expecting to have feedback like this). > > Calling it "strncmp_prefix()" when there is no "n" there makes no sense. > > So drop the "n" from the name. I originally did that, but then I thought strcmp() is a full compare, and 'n' denotes it is partial. But thinking about it more, yeah one would think it would need a parameter to represent 'n'. > > And honestly, maybe it would be better to use a different naming > pattern entirely, and avoid the crazy non-boolean "str*cmp()" model. > That thing is useful for search comparisons (where "before/after" > matters), but it's horrible for the actual "is this true or not", > where the code ends up being > > if (!strcmp_prefix(a, "prefix")) { > // This is the "Yes, prefix matched" case, despite the > "if !" syntax > > which is just confusing. > > So I'd really prefer more of a > > #define has_prefix(str, prefix) ... I like changing the name. > > model that actually returns a boolean (true if it has a prefix, false > if it doesn't), rather than use the "str*cmp" naming and return > values. Actually, instead of returning a bool, it can return the length if it matches. Reason being, there's several locations that does something like: while (...) { len = strlen("const"); if (strncmp(str, "const", len) == 0) { str += len; break; } } And I was having trouble thinking about how to deal with these. But if we have a has_prefix() that returns the length on match then we can do: if ((len = has_prefix(str, "const")) { str += len; > > (But I agree that *if* you use the "strcmp" naming, then you do need > to hold to the traditional strcmp return value semantics). > > Hmm? > > Finally, I also suspect that your helper might be slightly fragile. > Doing sizeof() seems broken. I could see somebody using some prefix > define for arrays with constant sizes, and doing > >#define PFX1 "cpp\0" >#define PFX2 "c\0\0\0" >#define PFX3 "h\0\0\0" > > or similar. Also, is there a reason you use "" for the constant That was left over in my original tests in userspace. When I first tried it with __builtin_constant_p() I got an error, and added the '&', but then fixed something else. The something else was what actually caused the error, but since it didn't complain (and past all my tests), I left in the '&'. > test? I don't see that pattern anywhere else. Plus it looks entirely > wrong without the parenthesis (ie "&(prefix)" to make sure we group > things right). > > So a lot of small problems, but the naming one is big. OK, what about if we just use strlen() and say that this macro is not safe for parameters with side effects. -- Steve diff --git a/include/linux/string.h b/include/linux/string.h index 27d0482e5e05..f9d274a81276 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -14,6 +14,29 @@ extern void *memdup_user(const void __user *, size_t); extern void *vmemdup_user(const void __user *, size_t); extern void *memdup_user_nul(const void __user *, size_t); +/** + * have_prefix - Test if a string has a given prefix + * @str: The string to test + * @prefix: The string to see if @str starts with + * + * IMPORTANT; @prefix must not have side effects (used more than once here) + * + * 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 has_prefix(). + * + * Returns: 0 if @str does not start with @prefix + strlen(@prefix) if @str does start with @prefix + */ +#define has_prefix(str, prefix) \ + ({ \ + int strcmp_prefix_len = strlen(prefix); \ + strncmp(str, prefix, strcmp_prefix_len) == 0 ? \ + strcmp_prefix_len : 0; \ + }) + /* * Include machine specific inline routines */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt wrote: > > I figured the best thing to do is to create a helper macro and place it > into include/linux/string.h. And go around and fix all the open coded > versions of it later. > > I plan on only applying this patch and updating the tracing hooks for > this merge window. And perhaps use it to fix some of the bugs that were > found. I like the helper function concept, but as they say about CS: "There is only one hard problem in computer science: naming and off-by-one errors". And this one has that problem. The name makes absolutely no sense. Calling it "strncmp_prefix()" when there is no "n" there makes no sense. So drop the "n" from the name. And honestly, maybe it would be better to use a different naming pattern entirely, and avoid the crazy non-boolean "str*cmp()" model. That thing is useful for search comparisons (where "before/after" matters), but it's horrible for the actual "is this true or not", where the code ends up being if (!strcmp_prefix(a, "prefix")) { // This is the "Yes, prefix matched" case, despite the "if !" syntax which is just confusing. So I'd really prefer more of a #define has_prefix(str, prefix) ... model that actually returns a boolean (true if it has a prefix, false if it doesn't), rather than use the "str*cmp" naming and return values. (But I agree that *if* you use the "strcmp" naming, then you do need to hold to the traditional strcmp return value semantics). Hmm? Finally, I also suspect that your helper might be slightly fragile. Doing sizeof() seems broken. I could see somebody using some prefix define for arrays with constant sizes, and doing #define PFX1 "cpp\0" #define PFX2 "c\0\0\0" #define PFX3 "h\0\0\0" or similar. Also, is there a reason you use "" for the constant test? I don't see that pattern anywhere else. Plus it looks entirely wrong without the parenthesis (ie "&(prefix)" to make sure we group things right). So a lot of small problems, but the naming one is big. Linus
[for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
From: Steven Rostedt A discussion came up in the trace triggers thread about converting a bunch of: strncmp(str, "const", sizeof("const") - 1) use cases into a helper macro. It started with: strncmp(str, const, sizeof(const) - 1) But then Joe Perches mentioned that if a const is not used, the sizeof() will be the size of a pointer, which can be bad. And that gcc will optimize strlen("const") into "sizeof("const") - 1". Thinking about this more, a quick grep in the kernel tree found several (thousands!) of cases that use this construct. A quick grep also revealed that there's probably several bugs in that use case. Some are that people forgot the "- 1" (which I found) and others could be that the constant for the sizeof is different than the constant (although, I haven't found any of those, but I also didn't look hard). I figured the best thing to do is to create a helper macro and place it into include/linux/string.h. And go around and fix all the open coded versions of it later. I plan on only applying this patch and updating the tracing hooks for this merge window. And perhaps use it to fix some of the bugs that were found. I was going to just use: strncmp(str, prefix, strlen(prefix)) but then realized that "prefix" is used twice, and will break if someone does something like: strncmp_prefix(str, ptr++); So instead I check with __builtin_constant_p() to see if the second parameter is truly a constant, which I use sizeof() anyway (why bother gcc to optimize it, if we already know it's a constant), and copy the parameter into a local variable and use that local variable for the non constant part (with strlen). Link: http://lkml.kernel.org/r/e3e754f2bd18e56eaa8baf79bee619316ebf4cfc.1545161087.git.tom.zanu...@linux.intel.com Link: http://lkml.kernel.org/r/20181219211615.2298e...@gandalf.local.home Cc: Joe Perches Cc: Tom Zanussi Cc: Linus Torvalds Cc: Greg Kroah-Hartman Signed-off-by: Steven Rostedt (VMware) --- include/linux/string.h | 22 ++ 1 file changed, 22 insertions(+) diff --git a/include/linux/string.h b/include/linux/string.h index 27d0482e5e05..a998a5ef 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t); extern void *vmemdup_user(const void __user *, size_t); extern void *memdup_user_nul(const void __user *, size_t); +/* + * 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 strncmp_prefix(). + */ +#define strncmp_prefix(str, prefix)\ + ({ \ + int strcmp_prefix_ret; \ + if (__builtin_constant_p()) {\ + strcmp_prefix_ret = \ + strncmp(str, prefix, sizeof(prefix) - 1); \ + } else {\ + typeof(prefix) strcmp_prefix = prefix; \ + strcmp_prefix_ret = \ + strncmp(str, strcmp_prefix, \ + strlen(strcmp_prefix)); \ + } \ + strcmp_prefix_ret; \ + }) + /* * Include machine specific inline routines */ -- 2.19.2