> Date: 2026-11-13 22:13:10+0100 > From: Alejandro Colomar > > > Date: 2026-11-13 21:57:24+0100 > > From: Alejandro Colomar > > > [...] > > > > - strstarts(): > > > > It could be improved by returning a boolean-like pointer, > > > > instead of a bool, but: > > > > > > > > - We may need to fix some callers, since some of them might > > > > need an explicit conversion with !!. It'd be risky to change > > > > the implementation without first verifying that. > > > > > > How many cases do we have that needs special attention? Can you run spatch to > > > grep those? > > > > I *think* the only ones I expect this would be necessary are functions > > that return a boolean-like int. > > > > // returns 1:true 0:false -1:error= > > int > > foo(...) > > { > > ... > > return !!strprefix(...); > > } > > I've searched for cases of this, and also for cases of assignment to > a variable that might be of type int (I expect this to be less > frequent). I haven't used spatch(1) so far, but just grep(1): > > $ grep -rn -e '= .*strstarts(' -e 'return .*strstarts(' > drivers/mtd/spi-nor/issi.c:141: return name && strstarts(name, "pm25lv"); > drivers/platform/x86/dell/dell-lis3lv02d.c:109: return strstarts(adap->name, "SMBus I801 adapter"); > arch/arm/kernel/module.c:25: return strstarts(name, ".init") || > arch/arm/kernel/module.c:32: return strstarts(name, ".exit") || > arch/powerpc/perf/imc-pmu.c:1026: return strstarts(event->pmu->name, "thread_imc"); > tools/perf/util/map.c:21: return strstarts(filename, "/data/app-lib/") || > tools/perf/util/annotate-arch/annotate-x86.c:144: if (arch->family >= 0x15 && (strstarts(ins1, "test") || > tools/perf/util/annotate-arch/annotate-x86.c:150: if (arch->family >= 0x19 && (strstarts(ins1, "add") || > tools/perf/util/annotate-arch/annotate-x86.c:200: arch->ins_is_fused = strstarts(cpuid, "AuthenticAMD") ? > tools/perf/util/symbol.c:854: if (e_machine == EM_S390 && strstarts(name, "psw_idle")) { > tools/perf/util/env.c:733: return strstarts(perf_arch, "ppc64") ? EM_PPC64 : EM_PPC; > tools/perf/util/env.c:1049: is_amd = env->cpuid && strstarts(env->cpuid, "AuthenticAMD") ? 1 : -1; > tools/perf/util/env.c:1072: is_intel = env->cpuid && strstarts(env->cpuid, "GenuineIntel") ? 1 : -1; > tools/perf/util/symbol.h:55: return strstarts(str, KLP_SYM_PREFIX); > tools/perf/builtin-trace.c:2272: if (*after == '=' && (strstarts(after + 1, "0x") || > tools/perf/perf.c:143: return (strstarts(var, header) && !strcmp(var + strlen(header), c->cmd)); > tools/perf/tests/symbols.c:155: if (dso != text_dso && strstarts(dso__short_name(dso), dso__short_name(text_dso))) > tools/testing/selftests/bpf/prog_tests/sockmap_redir.c:321: if (type == SK_SKB_INGRESS && strstarts(out, "v_")) > tools/objtool/klp-sympos.c:372: return strstarts(sym->sec->name, ".init"); > tools/objtool/klp-checksum.c:66: return strstarts(sym->sec->name, ".rodata.cst") && > tools/objtool/klp-diff.c:204: export->mod_ns = strstarts(namespace, "module:"); > tools/objtool/klp-diff.c:339: return strstarts(sym->name, ".L"); > tools/objtool/klp-diff.c:380: return strstarts(sec->name, ".discard."); > tools/objtool/klp-diff.c:422: return strstarts(sym->name, "__initcall__") || > kernel/module/main.c:1866: return strstarts(name, ".init"); > kernel/module/main.c:1871: return strstarts(name, ".exit"); With a multi-line search with pcre2grep(1) I've checked that I didn't miss any more complex cases. I'm quite confident that there's only one case that needs to be improved (the one below). Cheers, Alex > > From all these, only one case was using int. Here's a patch for making > it more robust: > > diff --git i/tools/perf/util/map.c w/tools/perf/util/map.c > index 41cdddc987ee..944f10d32796 100644 > --- i/tools/perf/util/map.c > +++ w/tools/perf/util/map.c > @@ -16,7 +16,7 @@ > #include "thread.h" > #include "vdso.h" > > -static inline int is_android_lib(const char *filename) > +static inline bool is_android_lib(const char *filename) > { > return strstarts(filename, "/data/app-lib/") || > strstarts(filename, "/system/lib/"); > > It's technically not necessary, since the '||' operator converts to > bool, but just in case someone removes one of those calls, using bool > will prevent a bug. > > All the other cases were either using 'bool', or didn't need a change > for other reasons. > > So, I can include this diff as a preparation patch in this set, and then > make strstarts() be > #define strstarts(s, prefix) strprefix(s, prefix) > > And then have a global search-and-replace for it. > > > Cheers, > Alex > > > > > > > > > > To avoid that risk, I prefer adding a new API, and > > > > deprecating strstarts() for new code. Over time, I could > > > > slowly migrate users from strstarts() to strprefix(). > > > > > > I believe this is unneeded step which may last for a few releases without even > > > guarantee that it will be finished. In such a case I would like to see a branch > > > with full conversion done before you start sending per-subsystem pieces out. > > > > Okay; I can do that. > > > > > > - The name isn't ideal, since "starts" doesn't specify whether > > > > it looks for a string that starts with a given prefix, or if > > > > it looks for a string that starts with any of the given > > > > characters. > > > > > > > > That is, by the name, I wouldn't be able to know whether > > > > strstarts implements > > > > > > > > bool strstarts(const char *s, const char *xxx) > > > > { > > > > return strspn(s, xxx); > > > > } > > > > or > > > > bool strstarts(const char *s, const char *xxx) > > > > { > > > > return strprefix(s, xxx); > > > > } > > > > > > I didn't get this. We should not really need an implementation details of > > > the API beneath. Documentation what should be read before use. Do we have > > > documentation for strstarts()? Is it clear? Maybe that what should be addressed > > > first? > > > > > > > - strends(): > > > > Same as with strstarts() but from the end. > > > > > > > > - str_has_prefix(): > > > > It was an attempt at strprefix(), but with a problematic > > > > approach. It's frozen because of trace code. > > > > > > > > - strprefix()/strsuffix(): > > > > I'd add them to slowly replace strstarts()/strends(). > > > > > > See above about "slowly"... > > > > > > > > > I prefer replacing strstarts()/strends() with strprefix()/strsuffix(). > > > > > > > > > > > > But if you prefer changing strstarts() to return a pointer, I'm also > > > > > > okay-ish with it. It's not a perfect name, but I can live with it. > > > > > > > > > > > > > However, after looking at the uses in the kernel, I prefer > > > > > > > to see the strprefix() be not implemented at all, just optimization against > > > > > > > strstarts()/strends() if required can be performed. > > > > > > > > > > > > I don't understand this last sentence. > > > > > > > > > > It's basically repeats the idea of having only one pair out of two. > > > > > > > > Ok. > > > > > > > > > With this series will be > > > > > > > > > > str_has_previx() > > > > > strstarts() > > > > > strends() > > > > > strprefix() > > > > > // potentially strsuffix() > > > > > > > > Yes. I'd like to replace strstarts()/strends() with > > > > strprefix()/strsuffix(), but prefer to do that slowly, to avoid > > > > introducing bugs due to missing explicit conversions to bool. > > > > > > > > Should I add notes that the strstarts()/strends() APIs are deprecated? > > > > > > Isn't Steven against that? I already forgot his points. > > > > As far as I understood, he was against deprecating str_has_prefix(), but > > we didn't yet talk about strstarts(). > > > > > Keeping them > > > just as aliases to strprefix()/strsuffix() perhaps the good start > > > (see below). > > > > Ok. > > > > > > > To me seems too many for the similar task. I understand that trace user is > > > > > special one, but does it use strstarts()/strends()? > > > > > > > > I didn't find any uses: > > > > > > > > $ git ls-files | grep kernel/trace/ | xargs grep strstarts > > > > $ git ls-files | grep kernel/trace/ | xargs grep strends > > > > > > > > > Despite on naming I think we just good to have only str_has*() and one of > > > > > strprefix()/strsuffix(), strstarts()/strends(). > > > > > > > > In the long term, I agree. But I think the transition should be > > > > careful. What do you think? > > > > > > Yes, it should take a few phases, first one perhaps to align strstarts() to not > > > rely on the returned value to be boolean. > > > > Ok. > > > > > In parallel with that, your current series can also be considered. Then > > > similar, if even required, to be done against strends(). > > > > Agree. > > > > > And one more phase > > > is to replace strstarts()/strends() with strprefix()/strsuffix() all over > > > the kernel. > > > > Ok. > > > > Thanks! > > > > > > Have a lovely night! > > Alex > > > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko > > > > > > > > > > > > > -- > > > > > > -- > --