[PATCH] tools: remove unnecessary x suffix in test strings

Kevin Locke posted 1 patch 3 months, 3 weeks ago
tools/debugging/kernel-chktaint | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] tools: remove unnecessary x suffix in test strings
Posted by Kevin Locke 3 months, 3 weeks ago
An "x" suffix was appended to test variable expansions, presumably to
avoid issues with empty strings in some old shells, or perhaps with the
intention of avoiding issues with dashes or other special characters
that an "x" prefix might have avoided.  In either case, POSIX ensures
that such protections are not necessary, and are unlikely to be
encountered in shells currently in use, as indicated by shellcheck
SC2268.

Remove the "x" suffixes which unnecessarily complicate the code.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Suggested-by: David Laight <david.laight.linux@gmail.com>
---

Thanks David, that's a good point about the x suffixes.  Since
shellcheck warns about the x prefixes (SC2268) and I'm not aware of any
shells currently in use which require them, I think they are safe to
remove to clean up the code a bit.  Here's a patch to do just that,
which can be applied on top of my previous patch.

Since -o is an XSI extension to POSIX, I've stuck with ||, but I think
you are right that x would not be required in that case either.

Thanks again,
Kevin


 tools/debugging/kernel-chktaint | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/debugging/kernel-chktaint b/tools/debugging/kernel-chktaint
index 051608a63d9f..051ac27b58eb 100755
--- a/tools/debugging/kernel-chktaint
+++ b/tools/debugging/kernel-chktaint
@@ -18,8 +18,8 @@ retrieved from /proc/sys/kernel/tainted on another system.
 EOF
 }
 
-if [ "$1"x != "x" ]; then
-	if  [ "$1"x = "--helpx" ] || [ "$1"x = "-hx" ] ; then
+if [ "$1" != "" ]; then
+	if  [ "$1" = "--help" ] || [ "$1" = "-h" ] ; then
 		usage
 		exit 1
 	elif  [ $1 -ge 0 ] 2>/dev/null ; then
-- 
2.51.0
Re: [PATCH] tools: remove unnecessary x suffix in test strings
Posted by David Laight 3 months, 3 weeks ago
On Thu, 16 Oct 2025 17:47:09 -0600
Kevin Locke <kevin@kevinlocke.name> wrote:

> An "x" suffix was appended to test variable expansions, presumably to
> avoid issues with empty strings in some old shells, or perhaps with the
> intention of avoiding issues with dashes or other special characters
> that an "x" prefix might have avoided.  In either case, POSIX ensures
> that such protections are not necessary, and are unlikely to be
> encountered in shells currently in use, as indicated by shellcheck
> SC2268.
> 
> Remove the "x" suffixes which unnecessarily complicate the code.
> 
> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> Suggested-by: David Laight <david.laight.linux@gmail.com>
> ---
> 
> Thanks David, that's a good point about the x suffixes.  Since
> shellcheck warns about the x prefixes (SC2268) and I'm not aware of any
> shells currently in use which require them,

The problems arise when $1 is (say) "-x", a simple LR parser will treat
[ -x = -x ] as a check for the file "=" being executable and then give
a syntax error for the second -x.
I can't imagine why shellcheck should warn about a leading x (or any other
character) provided field splitting is disabled (eg by "").
The leading x has definitely been needed in the past.

POSIX does require the three argument 'test' look for the middle argument
being an operator - but there might be historic shells that don't so that.
OTOH you are probably looking for code from the early 1980s!
But the POSIX spec (last time I read it) does point out the problems
with arbitrary strings being treated as operators causing complex expressions
be mis-parsed - which a leading x fixes.

> I think they are safe to
> remove to clean up the code a bit.  Here's a patch to do just that,
> which can be applied on top of my previous patch.
> 
> Since -o is an XSI extension to POSIX, I've stuck with ||, but I think
> you are right that x would not be required in that case either.

I'm not sure there are any common shells that don't support -o and -a.
They get used quite a lot.
I'm pretty sure they were supported by the pre-POSIX System-V shells
(or the /bin/[ program they ran).

	David

> 
> Thanks again,
> Kevin
> 
> 
>  tools/debugging/kernel-chktaint | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/debugging/kernel-chktaint b/tools/debugging/kernel-chktaint
> index 051608a63d9f..051ac27b58eb 100755
> --- a/tools/debugging/kernel-chktaint
> +++ b/tools/debugging/kernel-chktaint
> @@ -18,8 +18,8 @@ retrieved from /proc/sys/kernel/tainted on another system.
>  EOF
>  }
>  
> -if [ "$1"x != "x" ]; then
> -	if  [ "$1"x = "--helpx" ] || [ "$1"x = "-hx" ] ; then
> +if [ "$1" != "" ]; then
> +	if  [ "$1" = "--help" ] || [ "$1" = "-h" ] ; then
>  		usage
>  		exit 1
>  	elif  [ $1 -ge 0 ] 2>/dev/null ; then
Re: [PATCH] tools: remove unnecessary x suffix in test strings
Posted by Kevin Locke 3 months, 3 weeks ago
On Fri, 2025-10-17 at 15:12 +0100, David Laight wrote:
> On Thu, 16 Oct 2025 17:47:09 -0600 Kevin Locke <kevin@kevinlocke.name> wrote:
>> Remove the "x" suffixes which unnecessarily complicate the code.
> 
> The problems arise when $1 is (say) "-x", a simple LR parser will treat
> [ -x = -x ] as a check for the file "=" being executable and then give
> a syntax error for the second -x.
> I can't imagine why shellcheck should warn about a leading x (or any other
> character) provided field splitting is disabled (eg by "").
> The leading x has definitely been needed in the past.

Yep, it definitely has been.  The rationale on the wiki is that it's
not necessary for modern shells (and presumably that it unnecessarily
complicates the code): https://www.shellcheck.net/wiki/SC2268
However, it notes Zsh had issues as recently as 2015, which is not as
old as I would have expected.

> POSIX does require the three argument 'test' look for the middle argument
> being an operator - but there might be historic shells that don't so that.
> OTOH you are probably looking for code from the early 1980s!
> But the POSIX spec (last time I read it) does point out the problems
> with arbitrary strings being treated as operators causing complex expressions
> be mis-parsed - which a leading x fixes.

Good point.  I just reread it and can confirm that the current version
still notes issues mitigated by the X prefix with "historical shells"
and with greater than 4 argument cases:
https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html

>> I think they are safe to
>> remove to clean up the code a bit.  Here's a patch to do just that,
>> which can be applied on top of my previous patch.
>> 
>> Since -o is an XSI extension to POSIX, I've stuck with ||, but I think
>> you are right that x would not be required in that case either.
> 
> I'm not sure there are any common shells that don't support -o and -a.
> They get used quite a lot.
> I'm pretty sure they were supported by the pre-POSIX System-V shells
> (or the /bin/[ program they ran).

You are probably right.  I still remember when Debian policy allowed
them and posh added support in 2007/2008:
https://lists.debian.org/debian-devel/2006/11/msg00710.html
(I was corrected by Clint Adams about -a and -o being XSI extensions
some years before then when I noted posh lacked support, which is
probably why I still remember it.)

I find && and || more readable, but I'm open to changing it if you
feel strongly.

Do I understand correctly that you are in favor of using the x prefix?
I have a slight preference for leaving it off, but I'm open to adding
it if you (or others) feel strongly.

Thanks for the interesting discussion,
Kevin
Re: [PATCH] tools: remove unnecessary x suffix in test strings
Posted by David Laight 3 months, 3 weeks ago
On Fri, 17 Oct 2025 16:28:12 -0600
Kevin Locke <kevin@kevinlocke.name> wrote:

> On Fri, 2025-10-17 at 15:12 +0100, David Laight wrote:
> > On Thu, 16 Oct 2025 17:47:09 -0600 Kevin Locke <kevin@kevinlocke.name> wrote:  
> >> Remove the "x" suffixes which unnecessarily complicate the code.  
> > 
> > The problems arise when $1 is (say) "-x", a simple LR parser will treat
> > [ -x = -x ] as a check for the file "=" being executable and then give
> > a syntax error for the second -x.
> > I can't imagine why shellcheck should warn about a leading x (or any other
> > character) provided field splitting is disabled (eg by "").
> > The leading x has definitely been needed in the past.  
> 
> Yep, it definitely has been.  The rationale on the wiki is that it's
> not necessary for modern shells (and presumably that it unnecessarily
> complicates the code): https://www.shellcheck.net/wiki/SC2268
> However, it notes Zsh had issues as recently as 2015, which is not as
> old as I would have expected.

It doesn't really make much difference to the shell.
I really doubt you'll notice any difference in the time it takes to run.

> 
> > POSIX does require the three argument 'test' look for the middle argument
> > being an operator - but there might be historic shells that don't so that.
> > OTOH you are probably looking for code from the early 1980s!
> > But the POSIX spec (last time I read it) does point out the problems
> > with arbitrary strings being treated as operators causing complex expressions
> > be mis-parsed - which a leading x fixes.  
> 
> Good point.  I just reread it and can confirm that the current version
> still notes issues mitigated by the X prefix with "historical shells"
> and with greater than 4 argument cases:
> https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html

The fact that the 'greater than 4 argument case' can still require
a prefix character might be considered enough to make adding one all the
time 'good practise' even though it (probably) isn't actually needed.

While I wouldn't error not having a prefix, generating an error when
there is one seems wrong.

What does shellcheck do with [ "$a" = "$b" -o "$c" = "$d" ] ?
Or even [ "$a" "$b" "$c" "$d" "$e" "$f "$g" ] ??

> 
> >> I think they are safe to
> >> remove to clean up the code a bit.  Here's a patch to do just that,
> >> which can be applied on top of my previous patch.
> >> 
> >> Since -o is an XSI extension to POSIX, I've stuck with ||, but I think
> >> you are right that x would not be required in that case either.  
> > 
> > I'm not sure there are any common shells that don't support -o and -a.
> > They get used quite a lot.
> > I'm pretty sure they were supported by the pre-POSIX System-V shells
> > (or the /bin/[ program they ran).  
> 
> You are probably right.  I still remember when Debian policy allowed
> them and posh added support in 2007/2008:
> https://lists.debian.org/debian-devel/2006/11/msg00710.html
> (I was corrected by Clint Adams about -a and -o being XSI extensions
> some years before then when I noted posh lacked support, which is
> probably why I still remember it.)
> 
> I find && and || more readable, but I'm open to changing it if you
> feel strongly.

They get parsed entirely differently and are likely to be measurably slower.
Just FYI I tend not to use 'if' statements at all, just (eg):
	[ a = b ] && echo a == b

> Do I understand correctly that you are in favor of using the x prefix?
> I have a slight preference for leaving it off, but I'm open to adding
> it if you (or others) feel strongly.

I wouldn't take them out and consider shellcheck wrong, but the suffix
were just stupid.

	David

> Thanks for the interesting discussion,
> Kevin
Re: [PATCH] tools: remove unnecessary x suffix in test strings
Posted by Kevin Locke 3 months, 3 weeks ago
On Sun, 2025-10-19 at 11:17 +0100, David Laight wrote:
> On Fri, 17 Oct 2025 16:28:12 -0600 Kevin Locke <kevin@kevinlocke.name> wrote:
>> On Fri, 2025-10-17 at 15:12 +0100, David Laight wrote:
>>> On Thu, 16 Oct 2025 17:47:09 -0600 Kevin Locke <kevin@kevinlocke.name> wrote:  
>>>> Remove the "x" suffixes which unnecessarily complicate the code.  
>>> 
>>> The problems arise when $1 is (say) "-x", a simple LR parser will treat
>>> [ -x = -x ] as a check for the file "=" being executable and then give
>>> a syntax error for the second -x.
>>> I can't imagine why shellcheck should warn about a leading x (or any other
>>> character) provided field splitting is disabled (eg by "").
>>> The leading x has definitely been needed in the past.  
>> 
>> Yep, it definitely has been.  The rationale on the wiki is that it's
>> not necessary for modern shells (and presumably that it unnecessarily
>> complicates the code): https://www.shellcheck.net/wiki/SC2268
>> However, it notes Zsh had issues as recently as 2015, which is not as
>> old as I would have expected.
> 
> It doesn't really make much difference to the shell.
> I really doubt you'll notice any difference in the time it takes to run.

I agree.  However, I'm more concerned about readability and
understandability for developers less familiar with the quirks of old
shells.

>>> POSIX does require the three argument 'test' look for the middle argument
>>> being an operator - but there might be historic shells that don't so that.
>>> OTOH you are probably looking for code from the early 1980s!
>>> But the POSIX spec (last time I read it) does point out the problems
>>> with arbitrary strings being treated as operators causing complex expressions
>>> be mis-parsed - which a leading x fixes.  
>> 
>> Good point.  I just reread it and can confirm that the current version
>> still notes issues mitigated by the X prefix with "historical shells"
>> and with greater than 4 argument cases:
>> https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html
> 
> The fact that the 'greater than 4 argument case' can still require
> a prefix character might be considered enough to make adding one all the
> time 'good practise' even though it (probably) isn't actually needed.

That seems reasonable to me, although I'd prefer omitting x and
prohibiting >3 argument cases, which appears to be the route
shellcheck takes with SC2268 + SC2166.

> While I wouldn't error not having a prefix, generating an error when
> there is one seems wrong.
> What does shellcheck do with [ "$a" = "$b" -o "$c" = "$d" ] ?

It only produces SC2166 (discouraging -o).  However, for 
[ "x$a" = "x$b" -o "x$c" = "x$d" ] it also produces SC2268.

> Or even [ "$a" "$b" "$c" "$d" "$e" "$f "$g" ] ??

This, and [ "$a" "$b" "$c" ] and [ "$a" "$b" ] produce parser error
SC1073.  Unfortunately, this appears to be a long-standing shellcheck
issue:  https://github.com/koalaman/shellcheck/issues/1645

>> I find && and || more readable, but I'm open to changing it if you
>> feel strongly.
> 
> They get parsed entirely differently and are likely to be measurably slower.
> Just FYI I tend not to use 'if' statements at all, just (eg):
> 	[ a = b ] && echo a == b
> 
>> Do I understand correctly that you are in favor of using the x prefix?
>> I have a slight preference for leaving it off, but I'm open to adding
>> it if you (or others) feel strongly.
> 
> I wouldn't take them out and consider shellcheck wrong, but the suffix
> were just stupid.

Are you opposed to the patch I posted removing the suffixes?  I had
tagged you as Suggested-by due to misreading your first post.  If the
change is not something you'd suggest, I can repost without it.

Thanks,
Kevin
Re: [PATCH] tools: remove unnecessary x suffix in test strings
Posted by David Laight 3 months, 2 weeks ago
On Mon, 20 Oct 2025 14:18:32 -0600
Kevin Locke <kevin@kevinlocke.name> wrote:

> On Sun, 2025-10-19 at 11:17 +0100, David Laight wrote:
> > On Fri, 17 Oct 2025 16:28:12 -0600 Kevin Locke <kevin@kevinlocke.name> wrote:  
> >> On Fri, 2025-10-17 at 15:12 +0100, David Laight wrote:  
> >>> On Thu, 16 Oct 2025 17:47:09 -0600 Kevin Locke <kevin@kevinlocke.name> wrote:    
> >>>> Remove the "x" suffixes which unnecessarily complicate the code.    
> >>> 
> >>> The problems arise when $1 is (say) "-x", a simple LR parser will treat
> >>> [ -x = -x ] as a check for the file "=" being executable and then give
> >>> a syntax error for the second -x.
> >>> I can't imagine why shellcheck should warn about a leading x (or any other
> >>> character) provided field splitting is disabled (eg by "").
> >>> The leading x has definitely been needed in the past.    
> >> 
> >> Yep, it definitely has been.  The rationale on the wiki is that it's
> >> not necessary for modern shells (and presumably that it unnecessarily
> >> complicates the code): https://www.shellcheck.net/wiki/SC2268
> >> However, it notes Zsh had issues as recently as 2015, which is not as
> >> old as I would have expected.  
> > 
> > It doesn't really make much difference to the shell.
> > I really doubt you'll notice any difference in the time it takes to run.  
> 
> I agree.  However, I'm more concerned about readability and
> understandability for developers less familiar with the quirks of old
> shells.
> 
> >>> POSIX does require the three argument 'test' look for the middle argument
> >>> being an operator - but there might be historic shells that don't so that.
> >>> OTOH you are probably looking for code from the early 1980s!
> >>> But the POSIX spec (last time I read it) does point out the problems
> >>> with arbitrary strings being treated as operators causing complex expressions
> >>> be mis-parsed - which a leading x fixes.    
> >> 
> >> Good point.  I just reread it and can confirm that the current version
> >> still notes issues mitigated by the X prefix with "historical shells"
> >> and with greater than 4 argument cases:
> >> https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html  
> > 
> > The fact that the 'greater than 4 argument case' can still require
> > a prefix character might be considered enough to make adding one all the
> > time 'good practise' even though it (probably) isn't actually needed.  
> 
> That seems reasonable to me, although I'd prefer omitting x and
> prohibiting >3 argument cases, which appears to be the route
> shellcheck takes with SC2268 + SC2166.

Ugg.
I know the parser is 'problematic' but you need -o (and -a) to get
moderately efficient expression evaluation.
If shellcheck objects to those I'd guess it also objects to ( and ).

You really don't want to use [ ... ] && [ ... ] because it goes right
out to the command pipeline parser.
Not to mention the lack of grouping for a || b && c


> > While I wouldn't error not having a prefix, generating an error when
> > there is one seems wrong.
> > What does shellcheck do with [ "$a" = "$b" -o "$c" = "$d" ] ?  
> 
> It only produces SC2166 (discouraging -o).  However, for 
> [ "x$a" = "x$b" -o "x$c" = "x$d" ] it also produces SC2268.
> 
> > Or even [ "$a" "$b" "$c" "$d" "$e" "$f "$g" ] ??  
> 
> This, and [ "$a" "$b" "$c" ] and [ "$a" "$b" ] produce parser error
> SC1073.  Unfortunately, this appears to be a long-standing shellcheck
> issue:  https://github.com/koalaman/shellcheck/issues/1645
> 
> >> I find && and || more readable, but I'm open to changing it if you
> >> feel strongly.  
> > 
> > They get parsed entirely differently and are likely to be measurably slower.
> > Just FYI I tend not to use 'if' statements at all, just (eg):
> > 	[ a = b ] && echo a == b
> >   
> >> Do I understand correctly that you are in favor of using the x prefix?
> >> I have a slight preference for leaving it off, but I'm open to adding
> >> it if you (or others) feel strongly.  
> > 
> > I wouldn't take them out and consider shellcheck wrong, but the suffix
> > were just stupid.  
> 
> Are you opposed to the patch I posted removing the suffixes?  I had
> tagged you as Suggested-by due to misreading your first post.  If the
> change is not something you'd suggest, I can repost without it.

The suffixes are just wrong.
If the shell treats the first parameter to [ as an operator and $1 is "-"
it processes [ -x = ... and looks for a file "=".
Without the suffix the same happens when $1 is "-x".

A conformant shell won't do this for a 3-argument [.
But I also suspect that any conformant shell supports -o and -a.
The 7-argument [ definitely needs the prefix to protect against unexpected
operators.

So I still think shellcheck is just wrong here.
It ought to be checking FOR a prefix when there are 4 or more arguments.
It is one of those idioms you have to get used to.

But at the end of the day it is probably your call.

	David

> 
> Thanks,
> Kevin
Re: [PATCH] tools: remove unnecessary x suffix in test strings
Posted by Randy Dunlap 3 months, 3 weeks ago

On 10/16/25 4:47 PM, Kevin Locke wrote:
> An "x" suffix was appended to test variable expansions, presumably to
> avoid issues with empty strings in some old shells, or perhaps with the
> intention of avoiding issues with dashes or other special characters
> that an "x" prefix might have avoided.  In either case, POSIX ensures
> that such protections are not necessary, and are unlikely to be
> encountered in shells currently in use, as indicated by shellcheck
> SC2268.
> 
> Remove the "x" suffixes which unnecessarily complicate the code.
> 
> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> Suggested-by: David Laight <david.laight.linux@gmail.com>

Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

> ---
> 
> Thanks David, that's a good point about the x suffixes.  Since
> shellcheck warns about the x prefixes (SC2268) and I'm not aware of any
> shells currently in use which require them, I think they are safe to
> remove to clean up the code a bit.  Here's a patch to do just that,
> which can be applied on top of my previous patch.
> 
> Since -o is an XSI extension to POSIX, I've stuck with ||, but I think
> you are right that x would not be required in that case either.
> 
> Thanks again,
> Kevin
> 
> 
>  tools/debugging/kernel-chktaint | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/debugging/kernel-chktaint b/tools/debugging/kernel-chktaint
> index 051608a63d9f..051ac27b58eb 100755
> --- a/tools/debugging/kernel-chktaint
> +++ b/tools/debugging/kernel-chktaint
> @@ -18,8 +18,8 @@ retrieved from /proc/sys/kernel/tainted on another system.
>  EOF
>  }
>  
> -if [ "$1"x != "x" ]; then
> -	if  [ "$1"x = "--helpx" ] || [ "$1"x = "-hx" ] ; then
> +if [ "$1" != "" ]; then
> +	if  [ "$1" = "--help" ] || [ "$1" = "-h" ] ; then
>  		usage
>  		exit 1
>  	elif  [ $1 -ge 0 ] 2>/dev/null ; then

-- 
~Randy