tests: Handle $RANDOM not being supported by the shell

Message ID 1500029117-6387-1-git-send-email-peter.maydell@linaro.org
State Accepted
Commit 64f871e3c92ecd3c72a37b48bcf12812f0057734
Headers show

Commit Message

Peter Maydell July 14, 2017, 10:45 a.m.
In various places in our test makefiles and scripts we use the
shell $RANDOM to create a random number. This is a bash
specific extension, and doesn't work on other shells.
With dash the shell doesn't complain, it just effectively
always evaluates $RANDOM to 0:
  echo $((RANDOM + 32768))     => 32768

However, on NetBSD the shell will complain:
  "-sh: arith: syntax error: "RANDOM + 32768"

which means that "make check" fails.

Switch to using "${RANDOM:-0}" instead of $RANDOM,
which will portably either give us a random number or zero.
This means that on non-bash shells we don't get such
good test coverage via the MALLOC_PERTURB_ setting, but
we were already in that situation for non-bash shells.

Our only other uses of $RANDOM (in tests/qemu-iotests/check
and tests/qemu-iotests/162) are in shell scripts which use
a #!/bin/bash line so they are always run under bash.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 tests/Makefile.include | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.4

Comments

Kamil Rytarowski July 14, 2017, 11:27 a.m. | #1
On 14.07.2017 12:45, Peter Maydell wrote:
> In various places in our test makefiles and scripts we use the

> shell $RANDOM to create a random number. This is a bash

> specific extension, and doesn't work on other shells.


This is supported on other shells like ksh (Korn Shell), but still as an
extension.

> With dash the shell doesn't complain, it just effectively

> always evaluates $RANDOM to 0:

>   echo $((RANDOM + 32768))     => 32768

> 

> However, on NetBSD the shell will complain:

>   "-sh: arith: syntax error: "RANDOM + 32768"

> 


I will make sure whether this behavior is correct in our sh(1).

> which means that "make check" fails.

> 

> Switch to using "${RANDOM:-0}" instead of $RANDOM,

> which will portably either give us a random number or zero.

> This means that on non-bash shells we don't get such

> good test coverage via the MALLOC_PERTURB_ setting, but

> we were already in that situation for non-bash shells.

> 

> Our only other uses of $RANDOM (in tests/qemu-iotests/check

> and tests/qemu-iotests/162) are in shell scripts which use

> a #!/bin/bash line so they are always run under bash.

> 

> Suggested-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Kamil Rytarowski <n54@gmx.com>


> ---

>  tests/Makefile.include | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/tests/Makefile.include b/tests/Makefile.include

> index 6d6cb74..f6310d2 100644

> --- a/tests/Makefile.include

> +++ b/tests/Makefile.include

> @@ -826,7 +826,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \

>  		QTEST_QEMU_IMG=qemu-img$(EXESUF) \

> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \

>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER","$@")

>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) $(gcov-files-generic-y); do \

>  	  echo Gcov report for $$f:;\

> @@ -837,7 +837,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>  $(patsubst %, check-%, $(check-unit-y)): check-%: %

>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>  	$(call quiet-command, \

> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \

>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER","$*")

>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y) $(gcov-files-generic-y); do \

>  	  echo Gcov report for $$f:;\

>
Fam Zheng July 14, 2017, 11:41 a.m. | #2
On Fri, 07/14 11:45, Peter Maydell wrote:
> In various places in our test makefiles and scripts we use the

> shell $RANDOM to create a random number. This is a bash

> specific extension, and doesn't work on other shells.

> With dash the shell doesn't complain, it just effectively

> always evaluates $RANDOM to 0:

>   echo $((RANDOM + 32768))     => 32768

> 

> However, on NetBSD the shell will complain:

>   "-sh: arith: syntax error: "RANDOM + 32768"

> 

> which means that "make check" fails.

> 

> Switch to using "${RANDOM:-0}" instead of $RANDOM,

> which will portably either give us a random number or zero.

> This means that on non-bash shells we don't get such

> good test coverage via the MALLOC_PERTURB_ setting, but

> we were already in that situation for non-bash shells.

> 

> Our only other uses of $RANDOM (in tests/qemu-iotests/check

> and tests/qemu-iotests/162) are in shell scripts which use

> a #!/bin/bash line so they are always run under bash.

> 

> Suggested-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  tests/Makefile.include | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/tests/Makefile.include b/tests/Makefile.include

> index 6d6cb74..f6310d2 100644

> --- a/tests/Makefile.include

> +++ b/tests/Makefile.include

> @@ -826,7 +826,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \

>  		QTEST_QEMU_IMG=qemu-img$(EXESUF) \

> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \

>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER","$@")

>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) $(gcov-files-generic-y); do \

>  	  echo Gcov report for $$f:;\

> @@ -837,7 +837,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>  $(patsubst %, check-%, $(check-unit-y)): check-%: %

>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>  	$(call quiet-command, \

> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \


The whitespaces before $${RANDOM:-0} look a bit strange (unusual and unpaired).
Otherwise looks good. (I guess it provides a tiny bit of more readability given
how many non-whitespaces are already there before it.)

Regardlessly:

Reviewed-by: Fam Zheng <famz@redhat.com>


>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER","$*")

>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y) $(gcov-files-generic-y); do \

>  	  echo Gcov report for $$f:;\

> -- 

> 2.7.4

> 

>
Kamil Rytarowski July 14, 2017, 11:50 a.m. | #3
On 14.07.2017 13:27, Kamil Rytarowski wrote:
> On 14.07.2017 12:45, Peter Maydell wrote:

>> In various places in our test makefiles and scripts we use the

>> shell $RANDOM to create a random number. This is a bash

>> specific extension, and doesn't work on other shells.

> 

> This is supported on other shells like ksh (Korn Shell), but still as an

> extension.

> 

>> With dash the shell doesn't complain, it just effectively

>> always evaluates $RANDOM to 0:

>>   echo $((RANDOM + 32768))     => 32768

>>

>> However, on NetBSD the shell will complain:

>>   "-sh: arith: syntax error: "RANDOM + 32768"

>>

> 

> I will make sure whether this behavior is correct in our sh(1).

> 


This has been altered in NetBSD-current (+ 8.0BETA), sh(1) behaves like
dash.

>> which means that "make check" fails.

>>

>> Switch to using "${RANDOM:-0}" instead of $RANDOM,

>> which will portably either give us a random number or zero.

>> This means that on non-bash shells we don't get such

>> good test coverage via the MALLOC_PERTURB_ setting, but

>> we were already in that situation for non-bash shells.

>>

>> Our only other uses of $RANDOM (in tests/qemu-iotests/check

>> and tests/qemu-iotests/162) are in shell scripts which use

>> a #!/bin/bash line so they are always run under bash.

>>

>> Suggested-by: Eric Blake <eblake@redhat.com>

>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> 

> Reviewed-by: Kamil Rytarowski <n54@gmx.com>

> 

>> ---

>>  tests/Makefile.include | 4 ++--

>>  1 file changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/tests/Makefile.include b/tests/Makefile.include

>> index 6d6cb74..f6310d2 100644

>> --- a/tests/Makefile.include

>> +++ b/tests/Makefile.include

>> @@ -826,7 +826,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>>  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \

>>  		QTEST_QEMU_IMG=qemu-img$(EXESUF) \

>> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

>> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \

>>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER","$@")

>>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) $(gcov-files-generic-y); do \

>>  	  echo Gcov report for $$f:;\

>> @@ -837,7 +837,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>>  $(patsubst %, check-%, $(check-unit-y)): check-%: %

>>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>>  	$(call quiet-command, \

>> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

>> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \

>>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER","$*")

>>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y) $(gcov-files-generic-y); do \

>>  	  echo Gcov report for $$f:;\

>>

> 

>
Eric Blake July 14, 2017, 12:02 p.m. | #4
On 07/14/2017 06:27 AM, Kamil Rytarowski wrote:
> On 14.07.2017 12:45, Peter Maydell wrote:

>> In various places in our test makefiles and scripts we use the

>> shell $RANDOM to create a random number. This is a bash

>> specific extension, and doesn't work on other shells.

> 

> This is supported on other shells like ksh (Korn Shell), but still as an

> extension.

> 

>> With dash the shell doesn't complain, it just effectively

>> always evaluates $RANDOM to 0:

>>   echo $((RANDOM + 32768))     => 32768

>>

>> However, on NetBSD the shell will complain:

>>   "-sh: arith: syntax error: "RANDOM + 32768"

>>

> 

> I will make sure whether this behavior is correct in our sh(1).


The question is what your shell does for:

unset foo
echo $(( foo % 255 ))

if it reliably prints 0, then it would do the same when RANDOM is an
undefined (and not a magic) variable.  Presumably, where sh is going
wrong is that it is treating an undefined variable not as 0, but as a
syntax error, and you'd get that behavior regardless of whether RANDOM
is in the mix.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Eric Blake July 14, 2017, 12:03 p.m. | #5
On 07/14/2017 05:45 AM, Peter Maydell wrote:
> In various places in our test makefiles and scripts we use the

> shell $RANDOM to create a random number. This is a bash

> specific extension, and doesn't work on other shells.


As mentioned elsewhere, you could reword this to "this is an extension
in bash and some other shells, but not universal".  But I'm also okay
leaving it untouched.

> Switch to using "${RANDOM:-0}" instead of $RANDOM,

> which will portably either give us a random number or zero.

> This means that on non-bash shells we don't get such

> good test coverage via the MALLOC_PERTURB_ setting, but

> we were already in that situation for non-bash shells.

> 

> Our only other uses of $RANDOM (in tests/qemu-iotests/check

> and tests/qemu-iotests/162) are in shell scripts which use

> a #!/bin/bash line so they are always run under bash.

> 

> Suggested-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  tests/Makefile.include | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Stefan Hajnoczi July 17, 2017, 9:43 a.m. | #6
On Fri, Jul 14, 2017 at 11:45:17AM +0100, Peter Maydell wrote:
> In various places in our test makefiles and scripts we use the

> shell $RANDOM to create a random number. This is a bash

> specific extension, and doesn't work on other shells.

> With dash the shell doesn't complain, it just effectively

> always evaluates $RANDOM to 0:

>   echo $((RANDOM + 32768))     => 32768

> 

> However, on NetBSD the shell will complain:

>   "-sh: arith: syntax error: "RANDOM + 32768"

> 

> which means that "make check" fails.

> 

> Switch to using "${RANDOM:-0}" instead of $RANDOM,

> which will portably either give us a random number or zero.

> This means that on non-bash shells we don't get such

> good test coverage via the MALLOC_PERTURB_ setting, but

> we were already in that situation for non-bash shells.

> 

> Our only other uses of $RANDOM (in tests/qemu-iotests/check

> and tests/qemu-iotests/162) are in shell scripts which use

> a #!/bin/bash line so they are always run under bash.

> 

> Suggested-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  tests/Makefile.include | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Markus Armbruster July 20, 2017, 11:26 a.m. | #7
Peter Maydell <peter.maydell@linaro.org> writes:

> In various places in our test makefiles and scripts we use the

> shell $RANDOM to create a random number. This is a bash

> specific extension, and doesn't work on other shells.

> With dash the shell doesn't complain, it just effectively

> always evaluates $RANDOM to 0:

>   echo $((RANDOM + 32768))     => 32768

>

> However, on NetBSD the shell will complain:

>   "-sh: arith: syntax error: "RANDOM + 32768"

>

> which means that "make check" fails.

>

> Switch to using "${RANDOM:-0}" instead of $RANDOM,

> which will portably either give us a random number or zero.


Golden opportunity to implement https://www.xkcd.com/221/

> This means that on non-bash shells we don't get such

> good test coverage via the MALLOC_PERTURB_ setting, but

> we were already in that situation for non-bash shells.

>

> Our only other uses of $RANDOM (in tests/qemu-iotests/check

> and tests/qemu-iotests/162) are in shell scripts which use

> a #!/bin/bash line so they are always run under bash.

>

> Suggested-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  tests/Makefile.include | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/tests/Makefile.include b/tests/Makefile.include

> index 6d6cb74..f6310d2 100644

> --- a/tests/Makefile.include

> +++ b/tests/Makefile.include

> @@ -826,7 +826,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>  	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \

>  		QTEST_QEMU_IMG=qemu-img$(EXESUF) \

> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \

>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER","$@")

>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) $(gcov-files-generic-y); do \

>  	  echo Gcov report for $$f:;\

> @@ -837,7 +837,7 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)

>  $(patsubst %, check-%, $(check-unit-y)): check-%: %

>  	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)

>  	$(call quiet-command, \

> -		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

> +		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \

>  		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER","$*")

>  	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y) $(gcov-files-generic-y); do \

>  	  echo Gcov report for $$f:;\


A possible alternative might be putting

    export RANDOM = 4    # chosen by a fair dice roll

into the makefile.

There's another occurence in tests/qemu-iotests/162.
Peter Maydell July 20, 2017, 3:34 p.m. | #8
On 14 July 2017 at 11:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> In various places in our test makefiles and scripts we use the

> shell $RANDOM to create a random number. This is a bash

> specific extension, and doesn't work on other shells.

> With dash the shell doesn't complain, it just effectively

> always evaluates $RANDOM to 0:

>   echo $((RANDOM + 32768))     => 32768

>

> However, on NetBSD the shell will complain:

>   "-sh: arith: syntax error: "RANDOM + 32768"

>

> which means that "make check" fails.

>

> Switch to using "${RANDOM:-0}" instead of $RANDOM,

> which will portably either give us a random number or zero.

> This means that on non-bash shells we don't get such

> good test coverage via the MALLOC_PERTURB_ setting, but

> we were already in that situation for non-bash shells.

>

> Our only other uses of $RANDOM (in tests/qemu-iotests/check

> and tests/qemu-iotests/162) are in shell scripts which use

> a #!/bin/bash line so they are always run under bash.

>

> Suggested-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  tests/Makefile.include | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

>


Applied, thanks.

-- PMM

Patch hide | download patch | download mbox

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6d6cb74..f6310d2 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -826,7 +826,7 @@  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
 	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
 		QTEST_QEMU_IMG=qemu-img$(EXESUF) \
-		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \
+		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \
 		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y) $(check-qtest-generic-y),"GTESTER","$@")
 	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y) $(gcov-files-generic-y); do \
 	  echo Gcov report for $$f:;\
@@ -837,7 +837,7 @@  $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
 $(patsubst %, check-%, $(check-unit-y)): check-%: %
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
 	$(call quiet-command, \
-		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \
+		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$(( $${RANDOM:-0} % 255 + 1))} \
 		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $*,"GTESTER","$*")
 	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$(subst tests/,,$*)-y) $(gcov-files-generic-y); do \
 	  echo Gcov report for $$f:;\