[PATCH v2 0/5] iotest.FilePath fixes and cleanups

[PATCH v2 0/5] iotest.FilePath fixes and cleanups

[Nir Soffer]

Fix some issues introduced when iotests.FilePaths was added and merge it back
into FilePath keeping the option to create multiple file names.

Changes since v1:
- Remove unwanted submodule change [Eric]
- Add Fixes: tag

v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2020-08/msg00773.html

Nir Soffer (5):
  qemu-iotests: Fix FilePaths cleanup
  qemu-iotests: Fix FilePaths docstring
  qemu-iotests: Support varargs syntax in FilePaths
  qemu-iotests: Merge FilePaths and FilePath
  qemu-iotests: Simplify FilePath __init__

 tests/qemu-iotests/194        |  4 +--
 tests/qemu-iotests/208        |  2 +-
 tests/qemu-iotests/222        |  2 +-
 tests/qemu-iotests/257        | 10 +++----
 tests/qemu-iotests/iotests.py | 50 +++++++++++++++++------------------
 5 files changed, 33 insertions(+), 35 deletions(-)

-- 
2.26.2

[PATCH v2 1/5] qemu-iotests: Fix FilePaths cleanup

[Nir Soffer]

If os.remove() fails to remove one of the paths, for example if the file
was removed by the test, the cleanup loop would exit silently, without
removing the rest of the files.

Fixes: de263986b5dc
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/iotests.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 717b5b652c..16a04df8a3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -468,11 +468,11 @@ class FilePaths:
         return self.paths
 
     def __exit__(self, exc_type, exc_val, exc_tb):
-        try:
-            for path in self.paths:
+        for path in self.paths:
+            try:
                 os.remove(path)
-        except OSError:
-            pass
+            except OSError:
+                pass
         return False
 
 class FilePath(FilePaths):
-- 
2.26.2

Re: [PATCH v2 1/5] qemu-iotests: Fix FilePaths cleanup

[Max Reitz]

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

On 21.08.20 01:54, Nir Soffer wrote:
> If os.remove() fails to remove one of the paths, for example if the file
> was removed by the test, the cleanup loop would exit silently, without
> removing the rest of the files.
> 
> Fixes: de263986b5dc
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[PATCH v2 2/5] qemu-iotests: Fix FilePaths docstring

[Nir Soffer]

When this class was extracted from FilePath, the docstring was not
updated for generating multiple files, and the example usage was
referencing unrelated file.

Fixes: de263986b5dc
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/iotests.py | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 16a04df8a3..f34a1d7ef1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -450,14 +450,16 @@ def file_pattern(name):
 
 class FilePaths:
     """
-    FilePaths is an auto-generated filename that cleans itself up.
+    Context manager generating multiple file names. The generated files are
+    removed when exiting the context.
 
-    Use this context manager to generate filenames and ensure that the file
-    gets deleted::
+    Example usage:
+
+        with FilePaths(['test.img', 'test.sock']) as (img_path, sock_path):
+            # Use img_path and sock_path here...
+
+        # img_path and sock_path are automatically removed here.
 
-        with FilePaths(['test.img']) as img_path:
-            qemu_img('create', img_path, '1G')
-        # migration_sock_path is automatically deleted
     """
     def __init__(self, names, base_dir=test_dir):
         self.paths = []
-- 
2.26.2

Re: [PATCH v2 2/5] qemu-iotests: Fix FilePaths docstring

[Max Reitz]

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

On 21.08.20 01:54, Nir Soffer wrote:
> When this class was extracted from FilePath, the docstring was not
> updated for generating multiple files, and the example usage was
> referencing unrelated file.
> 
> Fixes: de263986b5dc
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [PATCH v2 2/5] qemu-iotests: Fix FilePaths docstring

[Max Reitz]

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

On 21.08.20 01:54, Nir Soffer wrote:
> When this class was extracted from FilePath, the docstring was not
> updated for generating multiple files, and the example usage was
> referencing unrelated file.
> 
> Fixes: de263986b5dc
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 16a04df8a3..f34a1d7ef1 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -450,14 +450,16 @@ def file_pattern(name):
>  
>  class FilePaths:
>      """
> -    FilePaths is an auto-generated filename that cleans itself up.
> +    Context manager generating multiple file names. The generated files are
> +    removed when exiting the context.
>  
> -    Use this context manager to generate filenames and ensure that the file
> -    gets deleted::
> +    Example usage:
> +
> +        with FilePaths(['test.img', 'test.sock']) as (img_path, sock_path):

On second thought, this isn’t a great example because image files and
sockets should go into different base directories.

Max

> +            # Use img_path and sock_path here...
> +
> +        # img_path and sock_path are automatically removed here.
>  
> -        with FilePaths(['test.img']) as img_path:
> -            qemu_img('create', img_path, '1G')
> -        # migration_sock_path is automatically deleted
>      """
>      def __init__(self, names, base_dir=test_dir):
>          self.paths = []
> 



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

Re: [PATCH v2 2/5] qemu-iotests: Fix FilePaths docstring

[Nir Soffer]

On Tue, Aug 25, 2020 at 1:48 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 21.08.20 01:54, Nir Soffer wrote:
> > When this class was extracted from FilePath, the docstring was not
> > updated for generating multiple files, and the example usage was
> > referencing unrelated file.
> >
> > Fixes: de263986b5dc
> > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> > ---
> >  tests/qemu-iotests/iotests.py | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 16a04df8a3..f34a1d7ef1 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -450,14 +450,16 @@ def file_pattern(name):
> >
> >  class FilePaths:
> >      """
> > -    FilePaths is an auto-generated filename that cleans itself up.
> > +    Context manager generating multiple file names. The generated files are
> > +    removed when exiting the context.
> >
> > -    Use this context manager to generate filenames and ensure that the file
> > -    gets deleted::
> > +    Example usage:
> > +
> > +        with FilePaths(['test.img', 'test.sock']) as (img_path, sock_path):
>
> On second thought, this isn’t a great example because image files and
> sockets should go into different base directories.

Right, will improve it in v3.

>
> Max
>
> > +            # Use img_path and sock_path here...
> > +
> > +        # img_path and sock_path are automatically removed here.
> >
> > -        with FilePaths(['test.img']) as img_path:
> > -            qemu_img('create', img_path, '1G')
> > -        # migration_sock_path is automatically deleted
> >      """
> >      def __init__(self, names, base_dir=test_dir):
> >          self.paths = []
> >
>
>

[PATCH v2 3/5] qemu-iotests: Support varargs syntax in FilePaths

[Nir Soffer]

Accept variable number of names instead of a sequence:

    with FilePaths("a", "b", "c") as (a, b, c):

The disadvantage is that base_dir must be used as kwarg:

    with FilePaths("a", "b", base_dir=soc_dir) as (sock1, sock2):

But this is more clear and calling optional argument as positional
arguments is bad idea anyway.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/194        |  4 ++--
 tests/qemu-iotests/257        | 10 ++++------
 tests/qemu-iotests/iotests.py |  6 +++---
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index da7c4265ec..08389f474e 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -26,8 +26,8 @@ iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
 
 with iotests.FilePath('source.img') as source_img_path, \
      iotests.FilePath('dest.img') as dest_img_path, \
-     iotests.FilePaths(['migration.sock', 'nbd.sock'], iotests.sock_dir) as \
-         [migration_sock_path, nbd_sock_path], \
+     iotests.FilePaths('migration.sock', 'nbd.sock', base_dir=iotests.sock_dir) \
+        as (migration_sock_path, nbd_sock_path), \
      iotests.VM('source') as source_vm, \
      iotests.VM('dest') as dest_vm:
 
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index e1e6077219..a9aa65bbe3 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -275,10 +275,9 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
                         an incomplete backup. Testing limitations prevent
                         testing competing writes.
     """
-    with iotests.FilePaths(['img', 'bsync1', 'bsync2',
-                            'fbackup0', 'fbackup1', 'fbackup2']) as \
-                            (img_path, bsync1, bsync2,
-                             fbackup0, fbackup1, fbackup2), \
+    with iotests.FilePaths(
+            'img', 'bsync1', 'bsync2', 'fbackup0', 'fbackup1', 'fbackup2') as \
+            (img_path, bsync1, bsync2, fbackup0, fbackup1, fbackup2), \
          iotests.VM() as vm:
 
         mode = "Mode {:s}; Bitmap Sync {:s}".format(msync_mode, bsync_mode)
@@ -441,8 +440,7 @@ def test_backup_api():
     """
     Test malformed and prohibited invocations of the backup API.
     """
-    with iotests.FilePaths(['img', 'bsync1']) as \
-         (img_path, backup_path), \
+    with iotests.FilePaths('img', 'bsync1') as (img_path, backup_path), \
          iotests.VM() as vm:
 
         log("\n=== API failure tests ===\n")
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f34a1d7ef1..cdcdc2ad8b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -455,13 +455,13 @@ class FilePaths:
 
     Example usage:
 
-        with FilePaths(['test.img', 'test.sock']) as (img_path, sock_path):
+        with FilePaths('test.img', 'test.sock') as (img_path, sock_path):
             # Use img_path and sock_path here...
 
         # img_path and sock_path are automatically removed here.
 
     """
-    def __init__(self, names, base_dir=test_dir):
+    def __init__(self, *names, base_dir=test_dir):
         self.paths = []
         for name in names:
             self.paths.append(os.path.join(base_dir, file_pattern(name)))
@@ -482,7 +482,7 @@ class FilePath(FilePaths):
     FilePath is a specialization of FilePaths that takes a single filename.
     """
     def __init__(self, name, base_dir=test_dir):
-        super(FilePath, self).__init__([name], base_dir)
+        super(FilePath, self).__init__(name, base_dir=base_dir)
 
     def __enter__(self):
         return self.paths[0]
-- 
2.26.2

Re: [PATCH v2 3/5] qemu-iotests: Support varargs syntax in FilePaths

[Max Reitz]

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

On 21.08.20 01:54, Nir Soffer wrote:
> Accept variable number of names instead of a sequence:
> 
>     with FilePaths("a", "b", "c") as (a, b, c):
> 
> The disadvantage is that base_dir must be used as kwarg:
> 
>     with FilePaths("a", "b", base_dir=soc_dir) as (sock1, sock2):
> 
> But this is more clear and calling optional argument as positional
> arguments is bad idea anyway.
> 
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  tests/qemu-iotests/194        |  4 ++--
>  tests/qemu-iotests/257        | 10 ++++------
>  tests/qemu-iotests/iotests.py |  6 +++---
>  3 files changed, 9 insertions(+), 11 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[PATCH v2 4/5] qemu-iotests: Merge FilePaths and FilePath

[Nir Soffer]

FilePath creates now one temporary file:

    with FilePath("a") as a:

Or more:

    with FilePath("a", "b", "c") as (a, b, c):

This is also the behavior of the file_path() helper, used by some of the
tests. Now we have only 2 helpers for creating temporary files instead
of 3.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/194        |  2 +-
 tests/qemu-iotests/208        |  2 +-
 tests/qemu-iotests/222        |  2 +-
 tests/qemu-iotests/257        |  4 ++--
 tests/qemu-iotests/iotests.py | 23 +++++++++++------------
 5 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 08389f474e..7a4863ab18 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -26,7 +26,7 @@ iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
 
 with iotests.FilePath('source.img') as source_img_path, \
      iotests.FilePath('dest.img') as dest_img_path, \
-     iotests.FilePaths('migration.sock', 'nbd.sock', base_dir=iotests.sock_dir) \
+     iotests.FilePath('migration.sock', 'nbd.sock', base_dir=iotests.sock_dir) \
         as (migration_sock_path, nbd_sock_path), \
      iotests.VM('source') as source_vm, \
      iotests.VM('dest') as dest_vm:
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 6cb642f821..54aa4be273 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -26,7 +26,7 @@ iotests.script_initialize(supported_fmts=['generic'])
 
 with iotests.FilePath('disk.img') as disk_img_path, \
      iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
-     iotests.FilePath('nbd.sock', iotests.sock_dir) as nbd_sock_path, \
+     iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \
      iotests.VM() as vm:
 
     img_size = '10M'
diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 6602f6b4ba..14d67c875b 100755
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -49,7 +49,7 @@ remainder = [("0xd5", "0x108000",  "32k"), # Right-end of partial-left [1]
 
 with iotests.FilePath('base.img') as base_img_path, \
      iotests.FilePath('fleece.img') as fleece_img_path, \
-     iotests.FilePath('nbd.sock', iotests.sock_dir) as nbd_sock_path, \
+     iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \
      iotests.VM() as vm:
 
     log('--- Setting up images ---')
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index a9aa65bbe3..c80e06ae28 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -275,7 +275,7 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
                         an incomplete backup. Testing limitations prevent
                         testing competing writes.
     """
-    with iotests.FilePaths(
+    with iotests.FilePath(
             'img', 'bsync1', 'bsync2', 'fbackup0', 'fbackup1', 'fbackup2') as \
             (img_path, bsync1, bsync2, fbackup0, fbackup1, fbackup2), \
          iotests.VM() as vm:
@@ -440,7 +440,7 @@ def test_backup_api():
     """
     Test malformed and prohibited invocations of the backup API.
     """
-    with iotests.FilePaths('img', 'bsync1') as (img_path, backup_path), \
+    with iotests.FilePath('img', 'bsync1') as (img_path, backup_path), \
          iotests.VM() as vm:
 
         log("\n=== API failure tests ===\n")
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index cdcdc2ad8b..1b5cdd493e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -448,18 +448,23 @@ class Timeout:
 def file_pattern(name):
     return "{0}-{1}".format(os.getpid(), name)
 
-class FilePaths:
+class FilePath:
     """
     Context manager generating multiple file names. The generated files are
     removed when exiting the context.
 
     Example usage:
 
-        with FilePaths('test.img', 'test.sock') as (img_path, sock_path):
+        with FilePath('test.img', 'test.sock') as (img_path, sock_path):
             # Use img_path and sock_path here...
 
         # img_path and sock_path are automatically removed here.
 
+    For convenience, calling with one argument yields a single file instead of
+    a tuple with one item:
+
+        with FilePath("a") as a:
+
     """
     def __init__(self, *names, base_dir=test_dir):
         self.paths = []
@@ -467,7 +472,10 @@ class FilePaths:
             self.paths.append(os.path.join(base_dir, file_pattern(name)))
 
     def __enter__(self):
-        return self.paths
+        if len(self.paths) == 1:
+            return self.paths[0]
+        else:
+            return self.paths
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         for path in self.paths:
@@ -477,15 +485,6 @@ class FilePaths:
                 pass
         return False
 
-class FilePath(FilePaths):
-    """
-    FilePath is a specialization of FilePaths that takes a single filename.
-    """
-    def __init__(self, name, base_dir=test_dir):
-        super(FilePath, self).__init__(name, base_dir=base_dir)
-
-    def __enter__(self):
-        return self.paths[0]
 
 def file_path_remover():
     for path in reversed(file_path_remover.paths):
-- 
2.26.2

Re: [PATCH v2 4/5] qemu-iotests: Merge FilePaths and FilePath

[Max Reitz]

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

On 21.08.20 01:54, Nir Soffer wrote:
> FilePath creates now one temporary file:
> 
>     with FilePath("a") as a:
> 
> Or more:
> 
>     with FilePath("a", "b", "c") as (a, b, c):
> 
> This is also the behavior of the file_path() helper, used by some of the
> tests. Now we have only 2 helpers for creating temporary files instead
> of 3.
> 
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  tests/qemu-iotests/194        |  2 +-
>  tests/qemu-iotests/208        |  2 +-
>  tests/qemu-iotests/222        |  2 +-
>  tests/qemu-iotests/257        |  4 ++--
>  tests/qemu-iotests/iotests.py | 23 +++++++++++------------
>  5 files changed, 16 insertions(+), 17 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[PATCH v2 5/5] qemu-iotests: Simplify FilePath __init__

[Nir Soffer]

Use list comprehension instead of append loop.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/iotests.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1b5cdd493e..7ebd0bcc92 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -467,9 +467,8 @@ class FilePath:
 
     """
     def __init__(self, *names, base_dir=test_dir):
-        self.paths = []
-        for name in names:
-            self.paths.append(os.path.join(base_dir, file_pattern(name)))
+        self.paths = [os.path.join(base_dir, file_pattern(name))
+                      for name in names]
 
     def __enter__(self):
         if len(self.paths) == 1:
-- 
2.26.2

Re: [PATCH v2 5/5] qemu-iotests: Simplify FilePath __init__

[Max Reitz]

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

On 21.08.20 01:54, Nir Soffer wrote:
> Use list comprehension instead of append loop.
> 
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [PATCH v2 0/5] iotest.FilePath fixes and cleanups

[Kevin Wolf]

Am 21.08.2020 um 01:54 hat Nir Soffer geschrieben:
> Fix some issues introduced when iotests.FilePaths was added and merge it back
> into FilePath keeping the option to create multiple file names.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>