[U-Boot] [PATCH] if_changed: fix error handling

Luca Ceresoli luca at lucaceresoli.net
Wed Jun 6 07:49:07 UTC 2018


Hi Chris,

On 06/06/2018 05:23, Chris Packham wrote:
> Hi,
> 
> On Tue, Jun 5, 2018 at 2:08 AM Luca Ceresoli <luca at lucaceresoli.net> wrote:
>>
>> The commands in if_changed and if_changed_dep are concatenated with a
>> ';', thus any error in any command except the last one will be
>> silently ignored. This is particularly relevant for the actual
>> payload, cmd_$(1), whose errors should not be unnoticed.
>>
>> Fix by replacing the ';' with '&&'.
>>
>> Signed-off-by: Luca Ceresoli <luca at lucaceresoli.net>
>>
>> ---
>>
>> Note: I'm not aware of any situation in which this bug has any visible
>> effect. I noticed the problem while working on a different topic, but
>> later I did that job in a different way, not involving if_changed
>> usages. But this is a bug anyway, so let's fix it.
>> ---
>>  scripts/Kbuild.include | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
>> index 2c7918ad3721..f722f75611df 100644
>> --- a/scripts/Kbuild.include
>> +++ b/scripts/Kbuild.include
>> @@ -255,16 +255,16 @@ any-pThis is tricky, arereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
>>  # Execute command if command has changed or prerequisite(s) are updated.
>>  #
>>  if_changed = $(if $(strip $(any-prereq) $(arg-check)),                       \
>> -       @set -e;                                                             \
>> -       $(echo-cmd) $(cmd_$(1));                                             \
>> +       @set -e;                                                                 \
>> +       $(echo-cmd) $(cmd_$(1)) &&                                               \
>>         printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd)
>>
>>  # Execute the command and also postprocess generated .d dependencies file.
>>  if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ),                  \
>> -       @set -e;                                                             \
>> -       $(echo-cmd) $(cmd_$(1));                                             \
>> -       scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\
>> -       rm -f $(depfile);                                                    \
>> +       @set -e;                                                                 \
>> +       $(echo-cmd) $(cmd_$(1)) &&                                               \
>> +       scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp &&  \
>> +       rm -f $(depfile) &&                                                      \
>>         mv -f $(dot-target).tmp $(dot-target).cmd)
>>
>>  # Usage: $(call if_changed_rule,foo)
> 
> The 'set -e' should cause the sub-shell to exit immediately if any of
> the commands fail, regardless of the ';' vs '&&'.

Thanks for your comment, it triggered me to go deeper in understanding
the issue. Apologies for not having clarified it all in the first place.

Actually ';' and '&&' are not treated the same way. The bash manpage
states about 'set -e':

> The shell does not exit if the command that fails is [...] part of
> any command executed in a && or || list except the command following
> the final && or ||

The issue I had initially happened when I wrote something like this:

cmd_mkimage = test -r /no/such/file && \
	$(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \
        >$(MKIMAGEOUTPUT) $(if $(KBUILD_VERBOSE:0=), && cat
$(MKIMAGEOUTPUT))

If the 'test' command fails, set -e won't stop the build. But replacing
'&&' with ';' makes the build stop immediately as expected. So I guess
cmd_mkimage is just meant to be used with ';'.

> Assuming I'm wrong above, this patch should probably go to the kbuild
> mailing list http://vger.kernel.org/vger-lists.html#linux-kbuild

-- 
Luca


More information about the U-Boot mailing list