Messages in this thread | | | Date | Fri, 21 Dec 2018 19:00:39 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH v3] string.h: Add str_has_prefix() helper |
| |
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
| |