[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3] tests: Avoid non-portable 'echo -ARG'
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [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
signature.asc
Description: OpenPGP digital signature