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

[Markus Armbruster]

Sascha Silbe <address@hidden> writes:

> 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".

Are the problems serious enough to warrant rejecting the attempt to run
the tests directly outright?

Judging from your description, I'm inclined to "no".

> Signed-off-by: Sascha Silbe <address@hidden>
> Reviewed-by: Bo Tu <address@hidden>
> ---
> 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:
> +        sys.stderr.write('Please run this test via ./check\n')
> +        sys.exit(os.EX_USAGE)
> +
>      debug = '-d' in sys.argv
>      verbosity = 1
>      verify_image_format(supported_fmts)

Aha, you're not actually rejecting the attempt to run the tests
directly, you're merely requiring two environment variables.  That's
okay with me.  However, the commit message should explain it.  I'd also
rephrase the error message to something like

    <program_name>: TEST_DIR and QEMU_DEFAULT_MACHINE must be set in environment
    The easiest way to do that is running the test via the check script.