[PATCH v3] string.h: Add str_has_prefix() helper

[PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Note, gcc appears to optimize this when we make it into an always_inline
static function, which removes a lot of issues that a macro produces.

Link: http://lkml.kernel.org/r/e3e754f2bd18e56eaa8baf79bee619316ebf4cfc.1545161087.git.tom.zanussi@linux.intel.com
Link: http://lkml.kernel.org/r/20181219211615.2298e781@gandalf.local.home
Link: http://lkml.kernel.org/r/CAHk-=wg_sR-UEC1ggmkZpypOUYanL5CMX4R7ceuaV4QMf5jBtg@mail.gmail.com

Cc: Tom Zanussi <zanussi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Suggestions-by: Linus Torvalds <torvalds@linux-foundation.org>
Suggestions-by: Joe Perches <joe@perches.com>
Suggestions-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---

Changes since v2:

 - Rename the helper function to str_has_prefix()
    (Linus Torvalds and Joe Perches)

 - Use a static inline, as that appears to still optimize correctly
    (Andreas Schwab)

 include/linux/string.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..a37859d91b57 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -456,4 +456,24 @@ 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_ */
-- 
2.19.1

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Joe Perches]

On Fri, 2018-12-21 at 18:13 -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 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.
> 
> Note, gcc appears to optimize this when we make it into an always_inline
> static function, which removes a lot of issues that a macro produces.
> 
> Link: http://lkml.kernel.org/r/e3e754f2bd18e56eaa8baf79bee619316ebf4cfc.1545161087.git.tom.zanussi@linux.intel.com
> Link: http://lkml.kernel.org/r/20181219211615.2298e781@gandalf.local.home
> Link: http://lkml.kernel.org/r/CAHk-=wg_sR-UEC1ggmkZpypOUYanL5CMX4R7ceuaV4QMf5jBtg@mail.gmail.com
> 
> Cc: Tom Zanussi <zanussi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Suggestions-by: Linus Torvalds <torvalds@linux-foundation.org>
> Suggestions-by: Joe Perches <joe@perches.com>
> Suggestions-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> 
> Changes since v2:
> 
>  - Rename the helper function to str_has_prefix()
>     (Linus Torvalds and Joe Perches)
> 
>  - Use a static inline, as that appears to still optimize correctly
>     (Andreas Schwab)
> 
>  include/linux/string.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 27d0482e5e05..a37859d91b57 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -456,4 +456,24 @@ 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;
> +}

I believe this should be bool.

I don't find a use for non-zero assigned len value in the kernel
for strncmp and I believe the function should simply be:

static inline bool str_has_prefix(const char *str, const char prefix[])
{
	return !strncmp(str, prefix, strlen(prefix));
}

It's hard to believe __always_inline vs inline matters
for any single line function.

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

On Fri, 21 Dec 2018 15:19:33 -0800
Joe Perches <joe@perches.com> wrote:

> I believe this should be bool.
> 
> I don't find a use for non-zero assigned len value in the kernel
> for strncmp and I believe the function should simply be:
> 
> static inline bool str_has_prefix(const char *str, const char prefix[])
> {
> 	return !strncmp(str, prefix, strlen(prefix));
> }

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3bb2b3351e35..e4566b9c2553 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -172,8 +172,8 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
 		 * it's not worth the risk */
 		return -EINVAL;
 
-	if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
-		buf += sizeof(temp) - 1;
+	if ((len = str_has_prefix(buf, temp))) {
+		buf += len;
 		sdkp->cache_override = 1;
 	} else {
 		sdkp->cache_override = 0;

And there's more places like this.

> 
> It's hard to believe __always_inline vs inline matters
> for any single line function.

I've been burnt by gcc deciding to not inline single functions before.
Why take the chance? It also documents that I must be inlined, and not
just a hint.

-- Steve

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

On Fri, 21 Dec 2018 18:25:07 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 21 Dec 2018 15:19:33 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > I believe this should be bool.
> > 
> > I don't find a use for non-zero assigned len value in the kernel
> > for strncmp and I believe the function should simply be:
> > 
> > static inline bool str_has_prefix(const char *str, const char prefix[])
> > {
> > 	return !strncmp(str, prefix, strlen(prefix));
> > }  
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3bb2b3351e35..e4566b9c2553 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -172,8 +172,8 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
>  		 * it's not worth the risk */
>  		return -EINVAL;
>  
> -	if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
> -		buf += sizeof(temp) - 1;
> +	if ((len = str_has_prefix(buf, temp))) {
> +		buf += len;
>  		sdkp->cache_override = 1;
>  	} else {
>  		sdkp->cache_override = 0;
> 
> And there's more places like this.
>

Heck, even the tracing code would like this. From the code that started
this entire discussion:

-- Steve

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 9d590138f870..632410c6e6c0 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4342,12 +4342,13 @@ static int parse_actions(struct hist_trigger_data *hist_data)
 	unsigned int i;
 	int ret = 0;
 	char *str;
+	int len;
 
 	for (i = 0; i < hist_data->attrs->n_actions; i++) {
 		str = hist_data->attrs->action_str[i];
 
-		if (str_has_prefix(str, "onmatch(")) {
-			char *action_str = str + sizeof("onmatch(") - 1;
+		if ((len = str_has_prefix(str, "onmatch("))) {
+			char *action_str = str + len;
 
 			data = onmatch_parse(tr, action_str);
 			if (IS_ERR(data)) {
@@ -4355,8 +4356,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
 				break;
 			}
 			data->fn = action_trace;
-		} else if (str_has_prefix(str, "onmax(")) {
-			char *action_str = str + sizeof("onmax(") - 1;
+		} else if ((len = str_has_prefix(str, "onmax("))) {
+			char *action_str = str + len;
 
 			data = onmax_parse(action_str);
 			if (IS_ERR(data)) {

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Joe Perches]

On Fri, 2018-12-21 at 18:25 -0500, Steven Rostedt wrote:
> On Fri, 21 Dec 2018 15:19:33 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > I believe this should be bool.
> > 
> > I don't find a use for non-zero assigned len value in the kernel
> > for strncmp and I believe the function should simply be:
> > 
> > static inline bool str_has_prefix(const char *str, const char prefix[])
> > {
> > 	return !strncmp(str, prefix, strlen(prefix));
> > }
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
[]
> @@ -172,8 +172,8 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
>  		 * it's not worth the risk */
>  		return -EINVAL;
>  
> -	if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
> -		buf += sizeof(temp) - 1;
> +	if ((len = str_has_prefix(buf, temp))) {
> +		buf += len;

That's not really a use of the non-zero strncmp return value.

You are attempting an optimization not already done.
I also wonder if it's actually an optimization as the
return value may not be precomputed.

Also the assignment in the test isn't preferred style.

> And there's more places like this.

Any where the non-zero return value is actually used?

> > It's hard to believe __always_inline vs inline matters 
> > for any single line function.
> 
> I've been burnt by gcc deciding to not inline single functions before.

Complex single functions sure, but single line inlines?
I haven't seen that externed anywhere.

Today no inline function is marked __always_inline in
string.h

I don't doubt there should be some standardization
of inline vs __always_inline in the kernel, but this
right now seems different just for difference sake.

cheers, Joe

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

On Fri, 21 Dec 2018 15:44:41 -0800
Joe Perches <joe@perches.com> wrote:

> On Fri, 2018-12-21 at 18:25 -0500, Steven Rostedt wrote:
> > On Fri, 21 Dec 2018 15:19:33 -0800
> > Joe Perches <joe@perches.com> wrote:
> >   
> > > I believe this should be bool.
> > > 
> > > I don't find a use for non-zero assigned len value in the kernel
> > > for strncmp and I believe the function should simply be:
> > > 
> > > static inline bool str_has_prefix(const char *str, const char prefix[])
> > > {
> > > 	return !strncmp(str, prefix, strlen(prefix));
> > > }  
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c  
> []
> > @@ -172,8 +172,8 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
> >  		 * it's not worth the risk */
> >  		return -EINVAL;
> >  
> > -	if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
> > -		buf += sizeof(temp) - 1;
> > +	if ((len = str_has_prefix(buf, temp))) {
> > +		buf += len;  
> 
> That's not really a use of the non-zero strncmp return value.
> 
> You are attempting an optimization not already done.
> I also wonder if it's actually an optimization as the
> return value may not be precomputed.

Note, temp is this:

	static const char temp[] = "temporary ";

> 
> Also the assignment in the test isn't preferred style.

We could have two helper functions:

static __always_inline bool
str_has_prefix(const char *str, const char *prefix)
{
	return strncmp(str, prefix, strlen(prefix));
}

and a 

static __always_inline bool
str_has_prefix_len(const char *str, const char *prefix, unsigned int *len)
{
	*len = strlen(prefix);
	return strncmp(str, prefix, *len);
}

This was my original thought with the first patches. But when Linus
suggested changing the style from the strncmp() I thought it was a way
to encapsulate the two.

Either way, but yes, I do want a way to do the compare and calculate
the length all in one function. That even makes checking options easier
to get to:

	if (str_has_prefix_len(cmdline, "param=", &len)) {
		value = cmdline + len;


> 
> > And there's more places like this.  
> 
> Any where the non-zero return value is actually used?
> 
> > > It's hard to believe __always_inline vs inline matters 
> > > for any single line function.  
> > 
> > I've been burnt by gcc deciding to not inline single functions before.  
> 
> Complex single functions sure, but single line inlines?
> I haven't seen that externed anywhere.
> 
> Today no inline function is marked __always_inline in
> string.h
> 
> I don't doubt there should be some standardization
> of inline vs __always_inline in the kernel, but this
> right now seems different just for difference sake.

I got burnt by some crazy gcc config options making local_irq_save()
become a out of line function, and that cause crazy crap to happen with
the function tracer.

Now inlining here is just for guaranteeing that strlen() gets turned
into a constant for constant strings and wont do anything harmful if
that doesn't happen (but slightly slow things down). But again, it
doesn't hurt to have the __always_inline. Why are you so dead against
it? You haven't stated your rational for that.

-- Steve

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

On Fri, 21 Dec 2018 18:13:16 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> +static __always_inline int str_has_prefix(const char *str, const char *prefix)

I'm thinking it is cleaner to have two helper functions and have them
both return bool.

static __always_inline bool str_has_prefix(const char *str, const char *prefix)
{
	return !strncmp(str, prefix, strlen(prefix));
}

( I still like to keep the __always_inline, it doesn't hurt )

And I'll make a separate patch that adds:

static __always_inline bool
str_has_prefix_len(const char *str, const char *prefix, unsigned int *len)
{
	*len = strlen(prefix);
	return !strncmp(str, prefix, *len) ? *len : 0;
}

Everyone OK with that?

-- Steve

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Joe Perches]

On Fri, 2018-12-21 at 19:06 -0500, Steven Rostedt wrote:
> On Fri, 21 Dec 2018 18:13:16 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > +static __always_inline int str_has_prefix(const char *str, const char *prefix)
> 
> I'm thinking it is cleaner to have two helper functions and have them
> both return bool.
> 
> static __always_inline bool str_has_prefix(const char *str, const char *prefix)
> {
> 	return !strncmp(str, prefix, strlen(prefix));
> }
> 
> ( I still like to keep the __always_inline, it doesn't hurt )
> 
> And I'll make a separate patch that adds:
> 
> static __always_inline bool
> str_has_prefix_len(const char *str, const char *prefix, unsigned int *len)
> {
> 	*len = strlen(prefix);
> 	return !strncmp(str, prefix, *len) ? *len : 0;
> }
> 
> Everyone OK with that? 

I guess so as you're the one doing all the work.

Trivia:

Shouldn't the latter function use __kernel_size_t
as that's the actual return type of strlen?

Thanks for keeping at it.

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

On Fri, 21 Dec 2018 16:32:58 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Dec 21, 2018, 16:06 Steven Rostedt <rostedt@goodmis.org wrote:
> 
> > On Fri, 21 Dec 2018 18:13:16
> >
> > And I'll make a separate patch that adds:
> >
> > static __always_inline bool
> > str_has_prefix_len(const char *str, const char *prefix, unsigned int *len)
> 
> 
> Why would this ever be a good idea? What's the advantage over returning the
> length?

Style?

I was just thinking that some people (like Joe) think it's in bad taste
to have:

	if ((len = str_has_prefix(str, "const"))) {

and it might look better to have:

	if (str_has_prefix_len(str, "const", &len)) {

Honestly, I'm good with either and don't really have a preference.

Let me know which one you prefer, and I'll work to get a patch out.

-- Steve

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Namhyung Kim]

Hi Steve,

On Fri, Dec 21, 2018 at 06:38:17PM -0500, Steven Rostedt wrote:
> On Fri, 21 Dec 2018 18:25:07 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 21 Dec 2018 15:19:33 -0800
> > Joe Perches <joe@perches.com> wrote:
> > 
> > > I believe this should be bool.
> > > 
> > > I don't find a use for non-zero assigned len value in the kernel
> > > for strncmp and I believe the function should simply be:
> > > 
> > > static inline bool str_has_prefix(const char *str, const char prefix[])
> > > {
> > > 	return !strncmp(str, prefix, strlen(prefix));
> > > }  
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 3bb2b3351e35..e4566b9c2553 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -172,8 +172,8 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
> >  		 * it's not worth the risk */
> >  		return -EINVAL;
> >  
> > -	if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
> > -		buf += sizeof(temp) - 1;
> > +	if ((len = str_has_prefix(buf, temp))) {
> > +		buf += len;
> >  		sdkp->cache_override = 1;
> >  	} else {
> >  		sdkp->cache_override = 0;
> > 
> > And there's more places like this.
> >
> 
> Heck, even the tracing code would like this. From the code that started
> this entire discussion:
> 
> -- Steve
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 9d590138f870..632410c6e6c0 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4342,12 +4342,13 @@ static int parse_actions(struct hist_trigger_data *hist_data)
>  	unsigned int i;
>  	int ret = 0;
>  	char *str;
> +	int len;
>  
>  	for (i = 0; i < hist_data->attrs->n_actions; i++) {
>  		str = hist_data->attrs->action_str[i];
>  
> -		if (str_has_prefix(str, "onmatch(")) {
> -			char *action_str = str + sizeof("onmatch(") - 1;
> +		if ((len = str_has_prefix(str, "onmatch("))) {
> +			char *action_str = str + len;

IMHO, returning (match) length might confuse people that it might
support partial match and returns the length actually matched rather
than full match.  If I knew it always returns the length of prefix (or
0) why it matters?  Using strlen() in the next line will have same
effect of compiler optimization for the constant strings, no?

		if (str_has_prefix(str, "onmatch(")) {
			char *action_str = str + strlen("onmatch(");

Thanks,
Namhyung


>  
>  			data = onmatch_parse(tr, action_str);
>  			if (IS_ERR(data)) {
> @@ -4355,8 +4356,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
>  				break;
>  			}
>  			data->fn = action_trace;
> -		} else if (str_has_prefix(str, "onmax(")) {
> -			char *action_str = str + sizeof("onmax(") - 1;
> +		} else if ((len = str_has_prefix(str, "onmax("))) {
> +			char *action_str = str + len;
>  
>  			data = onmax_parse(action_str);
>  			if (IS_ERR(data)) {

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 21 Dec 2018 16:32:58 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Fri, Dec 21, 2018, 16:06 Steven Rostedt <rostedt@goodmis.org wrote:
> > 
> > > On Fri, 21 Dec 2018 18:13:16
> > >
> > > And I'll make a separate patch that adds:
> > >
> > > static __always_inline bool
> > > str_has_prefix_len(const char *str, const char *prefix, unsigned int *len)
> > 
> > 
> > Why would this ever be a good idea? What's the advantage over returning the
> > length?
> 
> Style?
> 
> I was just thinking that some people (like Joe) think it's in bad taste
> to have:
> 
> 	if ((len = str_has_prefix(str, "const"))) {
> 
> and it might look better to have:
> 
> 	if (str_has_prefix_len(str, "const", &len)) {
> 
> Honestly, I'm good with either and don't really have a preference.

The first one is infinitely more readable and less ambiguous than a 
random series of arguments with unknown semantics for 'len': does 'len' 
have to be pre-initialized or does it always get set by the function, is 
the 'len' return always the same as the str_has_prefix_len() return value 
or is it a separate error code, etc.

I have no idea in what universe it's preferrable to pass it as an 
argument to a function.

We only punt return parameters to arguments when we are *forced* to, 
because there's too many of them, or there's some separate error and 
value path that cannot be encoded via any of the well-known pointer or 
integer encodings of errors, etc.

Thanks,

	Ingo

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

On Sat, 22 Dec 2018 18:28:18 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> >  
> >  	for (i = 0; i < hist_data->attrs->n_actions; i++) {
> >  		str = hist_data->attrs->action_str[i];
> >  
> > -		if (str_has_prefix(str, "onmatch(")) {
> > -			char *action_str = str + sizeof("onmatch(") - 1;
> > +		if ((len = str_has_prefix(str, "onmatch("))) {
> > +			char *action_str = str + len;  
> 
> IMHO, returning (match) length might confuse people that it might
> support partial match and returns the length actually matched rather
> than full match.  If I knew it always returns the length of prefix (or
> 0) why it matters?  Using strlen() in the next line will have same
> effect of compiler optimization for the constant strings, no?
> 
> 		if (str_has_prefix(str, "onmatch(")) {
> 			char *action_str = str + strlen("onmatch(");

The reason to return the length was to get rid of the need for
duplicating the strlen("xxxx") because it's a burden to keep the two
the same. Not only that, a lot of places just do "str + 7" because it's
easier to type.

Yes, it has the same effect on the compiler, but that's not what we are
trying to solve. We are trying to get rid of the duplication, because
that requires humans to get it right, and humans are not good at that.

-- Steve

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Steven Rostedt]

On Sat, 22 Dec 2018 11:58:45 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 21 Dec 2018 16:32:58 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >   
> > > On Fri, Dec 21, 2018, 16:06 Steven Rostedt <rostedt@goodmis.org wrote:
> > >   
> > > > On Fri, 21 Dec 2018 18:13:16
> > > >
> > > > And I'll make a separate patch that adds:
> > > >
> > > > static __always_inline bool
> > > > str_has_prefix_len(const char *str, const char *prefix, unsigned int *len)  
> > > 
> > > 
> > > Why would this ever be a good idea? What's the advantage over returning the
> > > length?  
> > 
> > Style?
> > 
> > I was just thinking that some people (like Joe) think it's in bad taste
> > to have:
> > 
> > 	if ((len = str_has_prefix(str, "const"))) {
> > 
> > and it might look better to have:
> > 
> > 	if (str_has_prefix_len(str, "const", &len)) {
> > 
> > Honestly, I'm good with either and don't really have a preference.  
> 
> The first one is infinitely more readable and less ambiguous than a 
> random series of arguments with unknown semantics for 'len': does 'len' 
> have to be pre-initialized or does it always get set by the function, is 
> the 'len' return always the same as the str_has_prefix_len() return value 
> or is it a separate error code, etc.
> 
> I have no idea in what universe it's preferrable to pass it as an 
> argument to a function.

I'm working on projects that prefer the second method. But these also
integrate a lot of C++ which using parameters for all I/O is the norm.

> 
> We only punt return parameters to arguments when we are *forced* to, 
> because there's too many of them, or there's some separate error and 
> value path that cannot be encoded via any of the well-known pointer or 
> integer encodings of errors, etc.

Like I said, I no longer have a strong preference and I'm happy with
either. It's like going back and forth between Europe and US and using
Celsius or Fahrenheit. There's really no difference in its use (unlike
meters / grams, which have the conversion advantage, temperature is
just a preference).

-- Steve

Re: [PATCH v3] string.h: Add str_has_prefix() helper

[Namhyung Kim]

On Sat, Dec 22, 2018 at 07:22:07AM -0500, Steven Rostedt wrote:
> On Sat, 22 Dec 2018 18:28:18 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > >  
> > >  	for (i = 0; i < hist_data->attrs->n_actions; i++) {
> > >  		str = hist_data->attrs->action_str[i];
> > >  
> > > -		if (str_has_prefix(str, "onmatch(")) {
> > > -			char *action_str = str + sizeof("onmatch(") - 1;
> > > +		if ((len = str_has_prefix(str, "onmatch("))) {
> > > +			char *action_str = str + len;  
> > 
> > IMHO, returning (match) length might confuse people that it might
> > support partial match and returns the length actually matched rather
> > than full match.  If I knew it always returns the length of prefix (or
> > 0) why it matters?  Using strlen() in the next line will have same
> > effect of compiler optimization for the constant strings, no?
> > 
> > 		if (str_has_prefix(str, "onmatch(")) {
> > 			char *action_str = str + strlen("onmatch(");
> 
> The reason to return the length was to get rid of the need for
> duplicating the strlen("xxxx") because it's a burden to keep the two
> the same. Not only that, a lot of places just do "str + 7" because it's
> easier to type.
> 
> Yes, it has the same effect on the compiler, but that's not what we are
> trying to solve. We are trying to get rid of the duplication, because
> that requires humans to get it right, and humans are not good at that.

Then I'm ok with that. :)

Thanks,
Namhyung