Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2787 bytes --]

On 14.04.2016 13:32, Sascha Silbe wrote:
> Running an iotests-based Python test directly might appear to work,
> but may fail in subtle ways and is insecure:
> 
> - It creates files with predictable file names in a world-writable
>   location (/var/tmp).
> 
> - Tests expect the environment to be set up by check. E.g. 041 and 055
>   may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
>   set. This can lead to false negatives.
> 
> Instead fail hard and tell the user we want to be run via "check".
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Reviewed-by: Bo Tu <tubo@linux.vnet.ibm.com>
> ---
> It's possible to fix iotests.py to work even outside of check, but
> that requires reimplementing parts of what check currently does. I'd
> prefer not to do that this late in the cycle.
> 
> It would be nice to have this in 2.6, just in case someone even tries
> running a Python-based test directly instead of using ./check like for
> the shell-based tests. But it's not crucial.
> ---
>  tests/qemu-iotests/iotests.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 0c0b533..8c7138f 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -46,7 +46,7 @@ if os.environ.get('QEMU_OPTIONS'):
>  
>  imgfmt = os.environ.get('IMGFMT', 'raw')
>  imgproto = os.environ.get('IMGPROTO', 'file')
> -test_dir = os.environ.get('TEST_DIR', '/var/tmp')
> +test_dir = os.environ.get('TEST_DIR')
>  output_dir = os.environ.get('OUTPUT_DIR', '.')
>  cachemode = os.environ.get('CACHEMODE')
>  qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
> @@ -441,6 +441,10 @@ def verify_quorum():
>  def main(supported_fmts=[], supported_oses=['linux']):
>      '''Run tests'''
>  
> +    if test_dir is None or qemu_default_machine is None:

I think checking test_dir would have been sufficient; this makes it look
like it might want to be a complete list of variables that have to be
set, but then "cachemode" is missing.

> +        sys.stderr.write('Please run this test via ./check\n')

Or not ./, for people who have separate build and source trees, you
don't want to invoke check in the source tree. ;-)

I'm not sure whether I'd want a v2 just because of this. It's
bike-shedding, but now that I think about it I guess I think I do.

So would you be so kind as to replace the "./check" by "the check
script"? :-)

(I don't particularly care about whether you change the condition used
in the if statement, though.)

Max

> +        sys.exit(os.EX_USAGE)
> +
>      debug = '-d' in sys.argv
>      verbosity = 1
>      verify_image_format(supported_fmts)
> 



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