From: Max Reitz <mreitz@redhat.com>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: edgar.iglesias@xilinx.com, stefanha@redhat.com,
qemu-block@nongnu.org, qemu-trivial@nongnu.org,
Kevin Wolf <kwolf@redhat.com>, Jiri Pirko <jiri@resnulli.us>,
"Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Subject: Re: [Qemu-devel] [PATCH v3] tests: Avoid non-portable 'echo -ARG'
Date: Mon, 3 Jul 2017 14:57:46 +0200 [thread overview]
Message-ID: <73fa75d7-3aa7-e125-af55-c9f383007a28@redhat.com> (raw)
In-Reply-To: <20170703123114.17103-1-eblake@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4554 bytes --]
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: qemu-trivial@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> 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 <mreitz@redhat.com>
> 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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]
next prev parent reply other threads:[~2017-07-03 12:58 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-03 12:31 [Qemu-devel] [PATCH v3] tests: Avoid non-portable 'echo -ARG' Eric Blake
2017-07-03 12:57 ` Max Reitz [this message]
2017-07-03 14:32 ` Eric Blake
2017-07-03 16:16 ` Max Reitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=73fa75d7-3aa7-e125-af55-c9f383007a28@redhat.com \
--to=mreitz@redhat.com \
--cc=eblake@redhat.com \
--cc=edgar.iglesias@gmail.com \
--cc=edgar.iglesias@xilinx.com \
--cc=jiri@resnulli.us \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-trivial@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).