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 <si...@linux.vnet.ibm.com> > Reviewed-by: Bo Tu <t...@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) >
signature.asc
Description: OpenPGP digital signature