Re: [Qemu-devel] [PATCH 04/10] qemu-iotests: cleanup and fix search for programs

[Eric Blake <eblake@redhat.com>]

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

On 09/12/2017 09:44 AM, Paolo Bonzini wrote:
> Instead of ./check failing when a binary is missing, we try each test
> case now and each one fails with tons of test case diffs.  Also, all the
> variables were initialized by "check" prior to "common" being sourced,
> and then (uselessly) checked for emptiness again in "check".
> 
> Centralize the search for programs in "common" (which will soon be
> one with "check"), including the "realpath" invocation which can be done
> just once in "check" rather than in the tests.
> 
> For qnio_server, move the detection to "common", simplifying
> set_prog_path to stop handling the unused second argument, and
> embedding the "realpath" pass.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qemu-iotests/check         | 41 ---------------------
>  tests/qemu-iotests/common        | 77 +++++++++++++++++++++++++++++++++++++---
>  tests/qemu-iotests/common.config | 61 +------------------------------
>  3 files changed, 73 insertions(+), 106 deletions(-)
> 

> +++ b/tests/qemu-iotests/common
> @@ -37,6 +37,17 @@ _full_platform_details()
>      echo "$os/$platform $host $kernel"
>  }
>  
> +# $1 = prog to look for
> +set_prog_path()
> +{
> +    p=`command -v $1 2> /dev/null`

Are we sure that $1 will always respond to -v sanely?

> +    if [ -n "$p" -a -x "$p" ]; then

[ ... -a ... ] should never be used; there are cases where it is
inherently ambiguous (even if it happens to work here, because we know
our shell is bash and because $p is not likely to contain text that
throws off the parser).  Please spell that either:

if [ -n "$p" ] && [ -x "$p" ]; then
if [[ -n $p && -x $p ]]; then

(that is, I don't mind if you rely on bash's [[]] in which case you can
use less typing, but if you stick to [], then you should use code that
can be pasted to other shells)

> +then
> +    if [ -x "$build_iotests/qemu" ]; then
> +        export QEMU_PROG="$build_iotests/qemu"
> +    elif [ -x "$build_root/$arch-softmmu/qemu-system-$arch" ]; then
> +        export QEMU_PROG="$build_root/$arch-softmmu/qemu-system-$arch"
> +    else
> +        pushd "$build_root" > /dev/null

Shouldn't you check for failure to change directories?

> +++ b/tests/qemu-iotests/common.config
> @@ -22,6 +22,7 @@ export LANG=C
>  PATH=".:$PATH"
>  
>  HOSTOS=`uname -s`
> +arch=`uname -m`

As long as we're touching this, should we prefer $() instead of ``?

>  
>  export PWD=`pwd`
>  
> @@ -30,28 +31,6 @@ export _QEMU_HANDLE=0
>  # make sure we have a standard umask
>  umask 022
>  
> -# $1 = prog to look for, $2* = default pathnames if not found in $PATH
> -set_prog_path()
> -{
> -    p=`command -v $1 2> /dev/null`
> -    if [ -n "$p" -a -x "$p" ]; then

Urrgh - your use of [ -a ] was pre-existing, and you just moved it.
Still worth fixing, though (either here or in a separate cleanup patch).

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


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