Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-29 Thread Michal Privoznik 

On 7/28/20 10:45 PM, Pavel Hrdina wrote:


Just a note, the file access testing is broken and it fails for a lot of
tests.


Yeah, I have patches for it, but I've never gotten around sending them. 
Will do today.


Michal

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Pavel Hrdina 
On Tue, Jul 28, 2020 at 09:32:57PM +0200, Pavel Hrdina wrote:
> 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 
> > > > > > > ---
> > > > > > >  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

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Pavel Hrdina 
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 
> > > > > > ---
> > > > > >  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

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Peter Krempa 
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 
> > > > > ---
> > > > >  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.

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Pavel Hrdina 
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 
> > > > ---
> > > >  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.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Pavel Hrdina 
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 
> > > ---
> > >  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.

Thanks

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Pino Toscano 
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 
> > ---
> >  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')

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Peter Krempa 
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 
> ---
>  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.


> +access_file = os.path.join(abs_builddir, 
> 'file-access-{0}.txt'.format(filename))
> +permitted_file = os.path.join(abs_srcdir, 'permitted_file_access.txt')
> +
> +os.environ['VIR_TEST_FILE_ACCESS_OUTPUT'] = access_file
> +
> +test = ' '.join(sys.argv[1:])
> +
> +ret = os.system(test)
> +
> +if ret != 0 or not os.is_file(access_file):
> +sys.exit(ret)
>  
>  known_actions = ["open", "fopen", "access", "stat", "lstat", "connect"]
>  
> @@ -120,6 +132,8 @@ for file in files:
>  print(": %s" % file["testname"], end="")
>  print("")
>  
> +os.remove(access_file)

Wouldn't it make sense to keep this file on failure?

> +
>  if err:
>  sys.exit(1)
>  sys.exit(0)

[libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-16 Thread Pavel Hrdina 
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 
---
 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))
+access_file = os.path.join(abs_builddir, 
'file-access-{0}.txt'.format(filename))
+permitted_file = os.path.join(abs_srcdir, 'permitted_file_access.txt')
+
+os.environ['VIR_TEST_FILE_ACCESS_OUTPUT'] = access_file
+
+test = ' '.join(sys.argv[1:])
+
+ret = os.system(test)
+
+if ret != 0 or not os.is_file(access_file):
+sys.exit(ret)
 
 known_actions = ["open", "fopen", "access", "stat", "lstat", "connect"]
 
@@ -120,6 +132,8 @@ for file in files:
 print(": %s" % file["testname"], end="")
 print("")
 
+os.remove(access_file)
+
 if err:
 sys.exit(1)
 sys.exit(0)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 641ed7b390b..04c37ccda2e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -16,18 +16,6 @@
 ## License along with this library.  If not, see
 ## .
 
-if WITH_LINUX
-check-access: file-access-clean
-   VIR_TEST_FILE_ACCESS=1 $(MAKE) $(AM_MAKEFLAGS) check
-   $(RUNUTF8) $(PYTHON) $(top_srcdir)/scripts/check-file-access.py \
-   $(abs_builddir)/test_file_access.txt \
-   $(abs_srcdir)/permitted_file_access.txt | sort -u
-
-file-access-clean:
-   > test_file_access.txt
-endif WITH_LINUX
-
-
 VALGRIND = valgrind --quiet --leak-check=full --trace-children=yes \

--trace-children-skip="*/tools/virsh","*/tests/commandhelper","/usr/bin/*" \
--suppressions=$(abs_srcdir)/.valgrind.supp
diff --git a/tests/meson.build b/tests/meson.build
index d6e0d2805d7..cf848678505 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -665,3 +665,11 @@ foreach name : test_scripts
   script = find_program(name)
   test(name, script, env: tests_env)
 endforeach
+
+add_test_setup(
+  'access',
+  env: [
+'VIR_TEST_FILE_ACCESS=1',
+  ],
+  exe_wrapper: [ meson_python_prog, check_file_access_prog ],
+)
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index e5dccae2a87..776493f0c5d 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -69,7 +69,7 @@ printFile(const char *file,
 output = VIR_FILE_ACCESS_DEFAULT;
 }
 
-if (!(fp = real_fopen(output, "a"))) {
+if (!(fp = real_fopen(output, "w"))) {
 fprintf(stderr, "Unable to open %s: %s\n", output, g_strerror(errno));
 abort();
 }
-- 
2.26.2