qemu-block
[Top][All Lists]
Advanced

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

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


From: Max Reitz
Subject: Re: [Qemu-block] [PATCH v3] tests: Avoid non-portable 'echo -ARG'
Date: Mon, 3 Jul 2017 14:57:46 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0

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).
> 
> Replace 'echo -n "..."' with 'printf %s "..."', and 'echo -e "..."'
> with 'printf %b "...\n"', with the optimization that the %s/%b
> argument can be omitted if the string being printed contains no
> substitutions that could result in '%' (trivial if there are no
> $ or `, but also possible when using things like $ret that are
> known to be numeric given the assignment to ret just above).
> 
> In the qemu-iotests check script, fix unusual shell quoting
> that would result in word-splitting if 'date' outputs a space.
> 
> In test 051, take an opportunity to shorten the line.
> 
> In test 068, get rid of a pointless second invocation of bash.
> 
> CC: address@hidden
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v3: use 'printf %s' in a few more places that substitute [Max]
> v2: be robust to potential % in substitutions
> ---
>  qemu-options.hx             |  4 ++--
>  tests/multiboot/run_test.sh | 10 +++++-----
>  tests/qemu-iotests/051      |  7 ++++---
>  tests/qemu-iotests/068      |  2 +-
>  tests/qemu-iotests/142      | 48 
> ++++++++++++++++++++++-----------------------
>  tests/qemu-iotests/171      | 14 ++++++-------
>  tests/qemu-iotests/check    | 18 ++++++++---------
>  tests/rocker/all            | 10 +++++-----
>  tests/tcg/cris/Makefile     |  8 ++++----
>  9 files changed, 61 insertions(+), 60 deletions(-)

[...]

> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 4b1c674..30af0eb 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check

[...]

> @@ -314,21 +314,21 @@ do
> 
>          if [ -f core ]
>          then
> -            echo -n " [dumped core]"
> +            printf " [dumped core]"
>              mv core $seq.core
>              err=true
>          fi
> 
>          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.

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".)

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]"
>                  err=true
>              fi
> 

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

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