2:32 p.m.
On Tue, Jul 28, 2020 at 09:13:41PM +0200, Peter Krempa wrote:
On Tue, Jul 28, 2020 at 18:48:18 +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 03:39:41PM +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:19:25PM +0200, Pino Toscano wrote:
> > > On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> > > > On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > > > > We need to modify check-file-access.py to be usable as wrapper
for
> > > > > libvirt tests. This way we can run the tests using this
command:
> > > > >
> > > > > meson test --setup access
> > > > >
> > > > > which will run all tests using check-file-access.py as a
wrapper.
> > > > >
> > > > > With autotools all file access are written into single file for
all
> > > > > tests and compared once the whole test suite is done.
> > > > >
> > > > > With Meson we will compare the file access after every single
test
> > > > > because it is used as wrapper now. That requires writing the
file
> > > > > access into separate files for every single test as they are
executed
> > > > > in parallel.
> > > > >
> > > > > Since the wrapper is used for all tests in Meson including tests
outside
> > > > > of tests directory we have to check for presence of the output
file.
> > > > > We should also cleanup after ourselves.
> > > > >
> > > > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> > > > > ---
> > > > > Makefile.am | 3 ---
> > > > > scripts/check-file-access.py | 24 +++++++++++++++++++-----
> > > > > tests/Makefile.am | 12 ------------
> > > > > tests/meson.build | 8 ++++++++
> > > > > tests/virtestmock.c | 2 +-
> > > > > 5 files changed, 28 insertions(+), 21 deletions(-)
> > > > >
> > > > > diff --git a/Makefile.am b/Makefile.am
> > > > > index 363c5cf66fd..d05a0c1a85a 100644
> > > > > --- a/Makefile.am
> > > > > +++ b/Makefile.am
> > > > > @@ -37,9 +37,6 @@ srpm: clean
> > > > >
> > > > > check-local: all tests
> > > > >
> > > > > -check-access: all
> > > > > - @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > > > > -
> > > > > dist-hook: gen-AUTHORS
> > > > >
> > > > > .PHONY: gen-AUTHORS
> > > > > diff --git a/scripts/check-file-access.py
b/scripts/check-file-access.py
> > > > > index aa120cafacf..f0e98f4b652 100755
> > > > > --- a/scripts/check-file-access.py
> > > > > +++ b/scripts/check-file-access.py
> > > > > @@ -21,15 +21,27 @@
> > > > > #
> > > > > #
> > > > >
> > > > > +import os
> > > > > +import random
> > > > > import re
> > > > > +import string
> > > > > import sys
> > > > >
> > > > > -if len(sys.argv) != 3:
> > > > > - print("syntax: %s ACCESS-FILE
PERMITTED-ACCESS-FILE")
> > > > > - sys.exit(1)
> > > > > +abs_builddir = os.environ.get('abs_builddir',
'')
> > > > > +abs_srcdir = os.environ.get('abs_srcdir', '')
> > > > >
> > > > > -access_file = sys.argv[1]
> > > > > -permitted_file = sys.argv[2]
> > > > > +filename = ''.join(random.choice(string.ascii_letters)
for _ in range(16))
> > > >
> > > > Umm, python doesn't have a 'mkostemp' equivalent? This
seems a bit
> > > > fishy.
> > >
> > > Sure, it is the tempfile module:
> > > https://docs.python.org/3/library/tempfile.html
> > >
> > > filename = tempfile.mkstemp(dir=abs_builddir,
prefix='file-access-', suffix='.txt')
> >
> > I'll look into it. When porting it I just looked for any solution as
> > this can be changed any time after the series is pushed.
>
> So I looked into it and the python script doesn't create the temporary
> file. The file is created by virtestmock.c if we run our test suite with
> file access check. The existence of the file is also used to check if
> there is something to be compared as not all of the tests needs to be
> check if they access some system files, for example all the check-* test
> cases.
>
> The tempfile is a nice module but useless in this case where we care
> about only getting a temporary file name. In order to use the module
> we would have to do something like this:
>
>
> fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-',
suffix='.txt')
> os.close(fd)
> os.unlink()
>
>
> which looks ugly. So I would rather keep it as it is.
Your algorithm is not robust enough so it could end up causing random
test failures. Unlikely but possible.
You can pass the filename to the test and let the mock append to the
existing file and then read it.
You are missing the point that the presence of the file is used later in
the script:
if ret != 0 or not os.path.exists(access_file):
sys.exit(ret)
the whole point is that only if the file exists the script will do the
actual check and compare the file-access-***.txt file.
If the file doesn't exist the script will fail with:
Traceback (most recent call last):
File "/home/phrdina/work/libvirt/scripts/check-file-access.py", line 52, in
<module>
with open(access_file, "r") as fh:
FileNotFoundError: [Errno 2] No such file or directory:
'file-access-IIyVhsUpnErLEkUm.txt'
If we don't mind running the whole script for test cases where the
access check doesn't make sense the example above can be changed to
this:
fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-',
suffix='.txt')
os.close(fd)
...
if ret != 0:
sys.exit(ret)
but at this point it doesn't make any difference and we can just go
with the first example where we remove the file as well.
Pavel