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

[Namhyung Kim <namhyung@kernel.org>]

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)) {