qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [PATCH v3] tests: Avoid non-portable 'echo -ARG'


From: Eric Blake
Subject: Re: [Qemu-trivial] [PATCH v3] tests: Avoid non-portable 'echo -ARG'
Date: Mon, 3 Jul 2017 09:32:33 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0

On 07/03/2017 07:57 AM, Max Reitz wrote:
> On 2017-07-03 14:31, Eric Blake wrote:
>> POSIX says that backslashes in the arguments to 'echo', as well as
>> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
>> people should favor 'printf' instead.  This is definitely true where
>> we do not control which shell is running (such as in makefile snippets
>> or in documentation examples).  But even for scripts where we
>> require bash (and therefore, where echo does what we want by default),
>> it is still possible to use 'shopt -s xpg_echo' to change bash's
>> behavior of echo.  And setting a good example never hurts when we are
>> not sure if a snippet will be copied from a bash-only script to a
>> general shell script (although I don't change the use of non-portable
>> \e for ESC when we know the running shell is bash).
>>

>>
>>          if [ -f $seq.notrun ]
>>          then
>> -            $timestamp || echo -n " [not run] "
>> -            $timestamp && echo " [not run]" && echo -n "        $seq -- "
>> +            $timestamp || printf " [not run] "
>> +            $timestamp && echo " [not run]" && printf "        $seq -- "
> 
> Everywhere else, $seq got its own %s, but not here. That's at least
> inconsistent.

True, and it's because $ is a bit easy to miss when I'm trying to
special case which $ are important.

> 
> I don't quite know why you're not simply using %s and %b everywhere
> there is a non-constant format string. Yes, if it is an exit code, it is
> pretty clear that it won't contain a %. If it's a return value from
> "date +%T", it is kind of, too (but then you have to know what that does
> in order to review the change -- and I myself had to consult the man
> page, to be honest). If they are variables named "img_offset" and
> "img_size", then you'd hope they are numbers, but in order to check you
> still have to look at the rest of the code (especially since they are
> embedded as strings into the JSON object).
> If you simply used %s and %b everywhere there is a $, reviewing would be
> absolutely trivial. Yes, it seems a bit unnecessary, but it does make
> reviewing much easier and I think since this is also about setting a
> good example, that would be a good example, IMHO.
> (Sure, reviewing this already is easy enough, but there still is a
> difference between "very easy" and "trivial".)

It's not every day a "trivial" patch gets to v4, but your arguments are
persuasive enough that I'll respin.

> 
> Although at this point I have to admit that reviewing a really trivial
> v4 would take me more time than to just write the following:
> 
> With a %s added to the printf above:
> 
> Reviewed-by: Max Reitz <address@hidden>
> 
>>              cat $seq.notrun
>>              notrun="$notrun $seq"
>>          else
>>              if [ $sts -ne 0 ]
>>              then
>> -                echo -n " [failed, exit status $sts]"
>> +                printf " [failed, exit status $sts]"

So, before I respin, let me double-check:

Here's a case where the context makes it obvious that $sts is numeric
(if not, the '[ $sts -ne 0 ]' would have failed); do you want the %s
here or not?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]