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
