On Fri, 21 Dec 2018 15:44:41 -0800 Joe Perches <[email protected]> wrote:
> On Fri, 2018-12-21 at 18:25 -0500, Steven Rostedt wrote: > > On Fri, 21 Dec 2018 15:19:33 -0800 > > Joe Perches <[email protected]> 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
