Re: [PATCH] board: toradex: Quote variables in `test` cmd expression

[Rasmus Villemoes <ravi@prevas.dk>]

On Fri, Apr 03 2026, Francesco Dolcini <francesco@dolcini.it> wrote:

> On Thu, Apr 02, 2026 at 05:30:29PM -0600, Tom Rini wrote:
>> On Wed, Apr 01, 2026 at 07:10:27AM +0200, Francesco Dolcini wrote:
>> > On Tue, Mar 31, 2026 at 10:10:10AM +0200, Franz Schnyder wrote:
>> > > From: Franz Schnyder <franz.schnyder@toradex.com>
>> > > 
>> > > With correct POSIX handling, unquoted empty variables can turn the
>> > > expression like
>> > > 	test -n ${fdtfile}
>> > > into
>> > > 	test -n
>> > > 
>> > > The POSIX handling for single argument `test` evaluates it as true,
>> > > so the fallback initialization will be skipped unexpectedly.
>> > > Quoting variable expansions in `test` expressions will always result in
>> > > correct behavior for empty and non-empty values.
>> > > This change was triggered by
>> > > commit 8b0619579b22 ("cmd: test: fix handling of single-argument form of test")
>> > > The aim is to have a less fragile codebase that is not dependent on a
>> > > quirk of the shell implementation.
>> > > 
>> > > Use quoted variable expansions in `test` expressions throughout.
>> > > 
>> > > Signed-off-by: Franz Schnyder <franz.schnyder@toradex.com>
>> > 
>> > Acked-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>> 
>> I've decided to go with:
>> https://patchwork.ozlabs.org/project/uboot/patch/20260330140106.401876-1-ravi@prevas.dk/
>> to make sure that everyone that might hit this problem doesn't end up
>> broken, thanks.
>
> I am wondering if you should apply this in any case. This change is generally correct.

I agree. And note that this really isn't anything to do with the 'test'
command per se, it's simply that using any variable whose value can be
empty or contain spaces is prone to produce something other than exactly
1 word, exactly as in userspace shell, and once the shell has done its
parsing and word splitting, the called command really has no way of
knowing that there's an empty argument missing.

> In the past when trying to move to some more modern shell as default stuff broke
> for us, we reverted the change without investigating. Maybe it was exactly the
> same issue.
>
> See https://lore.kernel.org/u-boot/20240111170418.GA7220@francesco-nb/

I don't think that was the same issue, but there are certainly differences
between the old and new parser when it comes to quoting and escaping,
and that 'env set set_apply_overlays ...' line looks quite prone to
being interpreted differently.

Rasmus