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

[Eric Blake]

On 07/02/2017 09:49 AM, Max Reitz wrote:
> 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? :-))

I can spin up a v3 that adds more %s/%b anywhere a $ appears where the
variable being printed is not obviously assigned in the immediately
preceding context.


>> +++ 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.

Indeed, any variable whose contents are provided from the filesystem
rather than directly by the script are suspects for abuse. In a
testsuite, it's often feasible to assume that abuse is not happening,
but being robust doesn't hurt.

>> +++ 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.

We'd be crazy to define TESTCASES with %, but I agree that the context
is not close, so being robust doesn't hurt.

Sounds like a v3 is coming up.

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

signature.asc
Description: OpenPGP digital signature