Re: [PATCH v5 7/7] string: Implement strstarts() with strprefix()

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6750 bytes --]

Hi Andy,

> Date: 2026-11-13 18:05:00+0200
> From: Andy Shevchenko <andriy.shevchenko@intel.com>
>
> On Fri, Nov 13, 2026 at 04:39:34PM +0100, Alejandro Colomar wrote:
> > > On Fri, Nov 13, 2026 at 03:59:46PM +0100, Alejandro Colomar wrote:
> > > > > On Fri, Nov 13, 2026 at 02:11:17PM +0100, Alejandro Colomar wrote:
> 
> ...
> 
> > > > > >  static inline bool strstarts(const char *str, const char *prefix)
> > > > > >  {
> > > > > > -	return strncmp(str, prefix, strlen(prefix)) == 0;
> > > > > > +	return strprefix(str, prefix);
> > > > > >  }
> > > > > 
> > > > > There is no sense to keep strstarts() at this point.
> > > > 
> > > > I agree.  Actually, it seems my guess that strprefix() would be
> > > > optimized out was wrong, because the kernel builds with
> > > > -fno-delete-null-pointer-checks and other flags that prevent this
> > > > optimization.
> > > > 
> > > > > Can it be simply redefinition?
> > > > 
> > > > I was thinking that we could just do a simple search and replace
> > > > s/strstarts/strends/, prepending !! in some cases if needed.
> > > > 
> > > > By 'redefinition', do you mean making strstarts() return a pointer
> > > > instead of a bool?  A problem with strstarts() is again the function
> > > > name: it doesn't seem to naturally fit with a function that returns a
> > > > pointer to the position.  It also doesn't clarify whether it searches
> > > > for a string, or for a list of characters (like for example strspn(3)).
> > > 
> > > We have str_has_*() for that?
> > 
> > I don't fully understand.  str_has_prefix() --which is the only
> > str_has_*() function, BTW-- is meant to be the same as strprefix(),
> > except for using an offset instead of a pointer.
> 
> You argued that strstarts() versus strprefix() is a naming problem,
> I answered that str_has_*() is for that kind of ops to solve naming issue.
> So, semantically str_has_*() should be like a boolean checker (or behave close
> to it, which it does).
> 
> > -  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(...);
	}

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

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]