command negation in modern hush parser

command negation in modern hush parser

[Rasmus Villemoes]

Hi

Today, I was bitten by u-boot shell not supporting the negating
operator, as in

  if ! some_command args ... ; then ... ; fi

So at first I thought about implementing a simple '!' command which,
similar to the the 'time' command, would just run the rest of the
arguments, but then negate the return code.

But then I looked at the modern hush parser code, which did seem to have
support for that in the shell itself. At least the "! command ..." gets
parsed, and there is code for rejecting "! ! command" which does seem to
be active, but unfortunately, the "! command" form doesn't actually work
as expected:

=> cli set modern

=> ! false
=> echo $?
1
=> ! true
=> echo $?
0
=> if ! false ; then echo "This should be printed" ; fi
=> if ! true ; then echo "This should _not_ be printed" ; fi
This should _not_ be printed
=> if ! echo "echoing always succeeds, so... " ; then echo "this should _not_ be printed" ; fi
echoing always succeeds, so... 
this should _not_ be printed
=> version
U-Boot 2026.04-rc3-00079-gb008b86fd180-dirty (Mar 11 2026 - 12:44:08 +0100)

gcc (GCC) 15.2.1 20260209
GNU ld (GNU Binutils) 2.46

I don't really grok the whole parser code, but just injecting a simple
printf at the start of done_pipe, it seems that that gets called twice
for each command, and the flag indicating inversion gets reset on the
first call, after which it doesn't affect the actual return code of the
command:

=> ! echo hi
done_pipe entered, followup 0, ctx->inverted = 1
done_pipe entered, followup 0, ctx->inverted = 0
hi

Any ideas?

Rasmus

Re: command negation in modern hush parser

[Anshul Dalal]

On Wed Mar 11, 2026 at 5:31 PM IST, Rasmus Villemoes wrote:
> Hi
>
> Today, I was bitten by u-boot shell not supporting the negating
> operator, as in
>
>   if ! some_command args ... ; then ... ; fi
>
> So at first I thought about implementing a simple '!' command which,
> similar to the the 'time' command, would just run the rest of the
> arguments, but then negate the return code.
>
> But then I looked at the modern hush parser code, which did seem to have
> support for that in the shell itself. At least the "! command ..." gets
> parsed, and there is code for rejecting "! ! command" which does seem to
> be active, but unfortunately, the "! command" form doesn't actually work
> as expected:
>
> => cli set modern
>
> => ! false
> => echo $?
> 1
> => ! true
> => echo $?
> 0
> => if ! false ; then echo "This should be printed" ; fi
> => if ! true ; then echo "This should _not_ be printed" ; fi
> This should _not_ be printed
> => if ! echo "echoing always succeeds, so... " ; then echo "this should _not_ be printed" ; fi
> echoing always succeeds, so... 
> this should _not_ be printed
> => version
> U-Boot 2026.04-rc3-00079-gb008b86fd180-dirty (Mar 11 2026 - 12:44:08 +0100)
>
> gcc (GCC) 15.2.1 20260209
> GNU ld (GNU Binutils) 2.46
>
> I don't really grok the whole parser code, but just injecting a simple
> printf at the start of done_pipe, it seems that that gets called twice
> for each command, and the flag indicating inversion gets reset on the
> first call, after which it doesn't affect the actual return code of the
> command:
>
> => ! echo hi
> done_pipe entered, followup 0, ctx->inverted = 1
> done_pipe entered, followup 0, ctx->inverted = 0
> hi
>
> Any ideas?

Hi Rasmus,

I was able to reproduce the issue and it looks like we might have missed
reverting the return code based on pi->pi_inverted inside run_pipe in
the one of the cases. The following diff fixes the issue for me:

	diff --git a/common/cli_hush_upstream.c b/common/cli_hush_upstream.c
	index 748ef60ac90..c01ba00e475 100644
	--- a/common/cli_hush_upstream.c
	+++ b/common/cli_hush_upstream.c
	@@ -10377,6 +10377,7 @@ static NOINLINE int run_pipe(struct pipe *pi)
		debug_printf_exec("run_pipe return -1 (%u children started)\n", pi->alive_cmds);
		return -1;
	 #else /* __U_BOOT__ */
	+       IF_HAS_KEYWORDS(if (pi->pi_inverted) rcode = !rcode;)
		debug_printf_exec("run_pipe return %d\n", rcode);
		return rcode;
	 #endif /* __U_BOOT__ */

Regards,
Anshul


>
> Rasmus

Re: command negation in modern hush parser

[Rasmus Villemoes]

On Thu, Mar 12 2026, Anshul Dalal <anshuld@ti.com> wrote:

> On Wed Mar 11, 2026 at 5:31 PM IST, Rasmus Villemoes wrote:
>>
>> => cli set modern
>>
>> => ! false
>> => echo $?
>> 1
>> => ! true
>> => echo $?
>> 0
>
> I was able to reproduce the issue and it looks like we might have missed
> reverting the return code based on pi->pi_inverted inside run_pipe in
> the one of the cases. The following diff fixes the issue for me:
>
> 	diff --git a/common/cli_hush_upstream.c b/common/cli_hush_upstream.c
> 	index 748ef60ac90..c01ba00e475 100644
> 	--- a/common/cli_hush_upstream.c
> 	+++ b/common/cli_hush_upstream.c
> 	@@ -10377,6 +10377,7 @@ static NOINLINE int run_pipe(struct pipe *pi)
> 		debug_printf_exec("run_pipe return -1 (%u children started)\n", pi->alive_cmds);
> 		return -1;
> 	 #else /* __U_BOOT__ */
> 	+       IF_HAS_KEYWORDS(if (pi->pi_inverted) rcode = !rcode;)
> 		debug_printf_exec("run_pipe return %d\n", rcode);
> 		return rcode;
> 	 #endif /* __U_BOOT__ */

Indeed, that seems to fix it, at least for the tests I could think of
throwing at it.

Will you send a proper patch?

Thanks,
Rasmus

Re: command negation in modern hush parser

[Anshul Dalal]

On Mon Mar 16, 2026 at 1:52 PM IST, Rasmus Villemoes wrote:
> On Thu, Mar 12 2026, Anshul Dalal <anshuld@ti.com> wrote:
>
>> On Wed Mar 11, 2026 at 5:31 PM IST, Rasmus Villemoes wrote:
>>>
>>> => cli set modern
>>>
>>> => ! false
>>> => echo $?
>>> 1
>>> => ! true
>>> => echo $?
>>> 0
>>
>> I was able to reproduce the issue and it looks like we might have missed
>> reverting the return code based on pi->pi_inverted inside run_pipe in
>> the one of the cases. The following diff fixes the issue for me:
>>
>> 	diff --git a/common/cli_hush_upstream.c b/common/cli_hush_upstream.c
>> 	index 748ef60ac90..c01ba00e475 100644
>> 	--- a/common/cli_hush_upstream.c
>> 	+++ b/common/cli_hush_upstream.c
>> 	@@ -10377,6 +10377,7 @@ static NOINLINE int run_pipe(struct pipe *pi)
>> 		debug_printf_exec("run_pipe return -1 (%u children started)\n", pi->alive_cmds);
>> 		return -1;
>> 	 #else /* __U_BOOT__ */
>> 	+       IF_HAS_KEYWORDS(if (pi->pi_inverted) rcode = !rcode;)
>> 		debug_printf_exec("run_pipe return %d\n", rcode);
>> 		return rcode;
>> 	 #endif /* __U_BOOT__ */
>
> Indeed, that seems to fix it, at least for the tests I could think of
> throwing at it.
>
> Will you send a proper patch?
>

Sure will do, thanks for validating the fix!

> Thanks,
> Rasmus