Re: [Qemu-devel] [PATCH v2] tests: Avoid non-portable 'echo -ARG'

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4804 bytes --]

On 2017-06-30 21:58, 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 is no
> $ or `, but also possible when using things like $ret that are
> known to be numeric given the assignment to ret just above).

Well, yes, possible, but it's no longer trivial... And just using '%s'
or '%b' in these cases would make reading the code simpler, in my opinion.

Sure, omitting it makes sense for constant format strings, but for
variable the cost outweighs the benefit, in my opinion.

(And since this is a bit supposed to go through qemu-trivial, it should
be trivial, right? :-))

> 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>
> 
> ---
> v2: be robust to potential % in substitutions [Max], rebase to master,
> shorten some long lines and an odd bash -c use
> 
> Add qemu-trivial in cc, although we may decide this is better through
> Max's block tree since it is mostly iotests related
> 
> ---
>  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..0a13df9 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check

[...]

> @@ -281,9 +281,9 @@ do
>          rm -f $seq.out.bad
>          lasttime=`sed -n -e "/^$seq /s/.* //p" <$TIMESTAMP_FILE`
>          if [ "X$lasttime" != X ]; then
> -                echo -n " ${lasttime}s ..."
> +                printf " ${lasttime}s ..."

You cannot prove this doesn't contain a %. In fact, I can very easily
put a % into the timestamp file and let printf print it here.

Sure, there shouldn't be one there, but on one hand it's still possible
and on the other, finding out that there shouldn't be a % there is very
much non-trivial.

>          else
> -                echo -n "        "        # prettier output with timestamps.
> +                printf "        "        # prettier output with timestamps.
>          fi
>          rm -f core $seq.notrun
> 

[...]

> diff --git a/tests/tcg/cris/Makefile b/tests/tcg/cris/Makefile
> index 6b3dba4..6888263 100644
> --- a/tests/tcg/cris/Makefile
> +++ b/tests/tcg/cris/Makefile
> @@ -150,17 +150,17 @@ check_addcv17.tst: crtv10.o sysv10.o
>  build: $(CRT) $(SYS) $(TESTCASES)
> 
>  check: $(CRT) $(SYS) $(TESTCASES)
> -	@echo -e "\nQEMU simulator."
> +	@printf "\nQEMU simulator.\n"
>  	for case in $(TESTCASES); do \
> -		echo -n "$$case "; \
> +		printf "$$case "; \

This is another rather non-trivial case: Checking that this doesn't
contain a % means reading through the whole list defining TESTCASES.

All in all, I'd really prefer just using %s and %b everywhere there is a
variable format string.

Max

>  		SIMARGS=; \
>  		case $$case in *v17*) SIMARGS="-cpu crisv17";; esac; \
>  		$(SIM) $$SIMARGS ./$$case; \
>  	done
>  check-g: $(CRT) $(SYS) $(TESTCASES)
> -	@echo -e "\nGDB simulator."
> +	@printf "\nGDB simulator.\n"
>  	@for case in $(TESTCASES); do \
> -		echo -n "$$case "; \
> +		printf "$$case "; \
>  		$(SIMG) $$case; \
>  	done
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]