| Message ID | 1500029117-6387-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 64f871e3c92ecd3c72a37b48bcf12812f0057734 |
| Headers | show |
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:;\ >
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 > >
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:;\ >> > >
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
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
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>
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.
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
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:;\
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