[PATCH v2 0/8] iotests: Fix 129 and expand 297’s reach

[PATCH v2 0/8] iotests: Fix 129 and expand 297’s reach

[Max Reitz]

v1 cover letter:
https://lists.nongnu.org/archive/html/qemu-block/2021-01/msg00254.html

Hi,

See the cover letter above for the main point of this series (it’s just
that all patch indices are shifted up by one in v2).

In addition to that, I’ve added patch 1 that makes some changes to 297
so it checks all Python files in qemu-iotests/ by default (with an
extensive skip list for now).  With Vladimir’s changes to test naming,
we’ll need to extend this to include files under qemu-iotests/tests/,
but that shouldn’t be a problem.

v2:
- Added patch 1 so 297 checks all Python files in qemu-iotests/ by
  default, skipping “only” files given in an explicit list.  This list
  is extremely long right now, but at least patch 8 makes it one entry
  shorter.

- Patch 5:
  - Use multi-line string concatenation without +
  - Drop block-job-cancel

- Patch 6:
  - Use multi-line string concatenation without +

- Patch 7:
  - Changed the commit message from implying that 'stop' shouldn’t drain
    the block job (it does and it should) to stating that 'stop'
    shouldn’t make the block job try to complete

- Patch 8:
  - Remove 129 from 297’s new skip list


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/8:[down] 'iotests/297: Allow checking all Python test files'
002/8:[----] [--] 'iotests: Move try_remove to iotests.py'
003/8:[----] [--] 'iotests/129: Remove test images in tearDown()'
004/8:[----] [--] 'iotests/129: Do not check @busy'
005/8:[0008] [FC] 'iotests/129: Use throttle node'
006/8:[0002] [FC] 'iotests/129: Actually test a commit job'
007/8:[----] [--] 'iotests/129: Limit mirror job's buffer size'
008/8:[0003] [FC] 'iotests/129: Clean up pylint and mypy complaints'


Max Reitz (8):
  iotests/297: Allow checking all Python test files
  iotests: Move try_remove to iotests.py
  iotests/129: Remove test images in tearDown()
  iotests/129: Do not check @busy
  iotests/129: Use throttle node
  iotests/129: Actually test a commit job
  iotests/129: Limit mirror job's buffer size
  iotests/129: Clean up pylint and mypy complaints

 tests/qemu-iotests/124        |  8 +---
 tests/qemu-iotests/129        | 74 +++++++++++++++++++++--------------
 tests/qemu-iotests/297        | 66 ++++++++++++++++++++++++++++---
 tests/qemu-iotests/297.out    |  7 +++-
 tests/qemu-iotests/iotests.py | 11 ++++--
 5 files changed, 119 insertions(+), 47 deletions(-)

-- 
2.29.2

[PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Max Reitz]

I.e., all Python files in the qemu-iotests/ directory.

Most files of course do not pass, so there is an extensive skip list for
now.  (The only files that do pass are 209, 254, 283, and iotests.py.)

(Alternatively, we could have the opposite, i.e. an explicit list of
files that we do want to check, but I think it is better to check files
by default.)

I decided to include the list of files checked in the reference output,
so we do not accidentally lose coverage of anything.  That means adding
new Python tests will require a change to 297.out, but that should not
be a problem.

On the other hand, I decided to hide mypy's "Success" lines from the
reference output, because they do not add anything useful.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/297     | 66 ++++++++++++++++++++++++++++++++++----
 tests/qemu-iotests/297.out |  6 +++-
 2 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 5c5420712b..b1a7d6d5e8 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then
     _notrun "mypy not found"
 fi
 
-pylint-3 --score=n iotests.py
+# TODO: Empty this list!
+skip_files=(
+    030 040 041 044 045 055 056 057 065 093 096 118 124 129 132 136 139 147 148
+    149 151 152 155 163 165 169 194 196 199 202 203 205 206 207 208 210 211 212
+    213 216 218 219 222 224 228 234 235 236 237 238 240 242 245 246 248 255 256
+    257 258 260 262 264 266 274 277 280 281 295 296 298 299 300 302 303 304 307
+    nbd-fault-injector.py qcow2.py qcow2_format.py qed.py
+)
 
-MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
-    --disallow-any-generics --disallow-incomplete-defs \
-    --disallow-untyped-decorators --no-implicit-optional \
-    --warn-redundant-casts --warn-unused-ignores \
-    --no-implicit-reexport iotests.py
+file_list=()
+for file in *; do
+    # Check files with a .py extension or a Python shebang
+    # (Unless they are in the skip_files list)
+    if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') ||
+                          (head -n 1 "$file" | grep -q '^#!.*python'))
+    then
+        skip_file=false
+        for skip in "${skip_files[@]}"; do
+            if [ "$skip" = "$file" ]; then
+                skip_file=true
+                break
+            fi
+        done
+
+        if ! $skip_file; then
+            file_list+=("$file")
+        fi
+    fi
+done
+
+# Emit list of all files that are checked so we will not accidentally
+# lose coverage
+echo 'Files to be checked:'
+
+file_list_str=''
+for file in "${file_list[@]}"; do
+    echo "  $file"
+done | sort
+
+# We can pass all files to pylint at once...
+pylint-3 --score=n "${file_list[@]}"
+
+# ...but mypy needs to be called once per file.  Otherwise, it will
+# interpret all given files as belonging together (i.e., they may not
+# both define the same classes, etc.; most notably, they must not both
+# define the __main__ module).
+for file in "${file_list[@]}"; do
+    mypy_output=$(
+        MYPYPATH=../../python/ mypy --warn-unused-configs \
+            --disallow-subclassing-any \
+            --disallow-any-generics --disallow-incomplete-defs \
+            --disallow-untyped-decorators --no-implicit-optional \
+            --warn-redundant-casts --warn-unused-ignores \
+            --no-implicit-reexport "$file" \
+            2>&1
+    )
+
+    if [ $? != 0 ]; then
+        echo "$mypy_output"
+    fi
+done
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
index 6acc843649..c5ebbf6a17 100644
--- a/tests/qemu-iotests/297.out
+++ b/tests/qemu-iotests/297.out
@@ -1,3 +1,7 @@
 QA output created by 297
-Success: no issues found in 1 source file
+Files to be checked:
+  209
+  254
+  283
+  iotests.py
 *** done
-- 
2.29.2

[PATCH v2 2/8] iotests: Move try_remove to iotests.py

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/124        |  8 +-------
 tests/qemu-iotests/iotests.py | 11 +++++++----
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3705cbb6b3..e40eeb50b9 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -22,6 +22,7 @@
 
 import os
 import iotests
+from iotests import try_remove
 
 
 def io_write_patterns(img, patterns):
@@ -29,13 +30,6 @@ def io_write_patterns(img, patterns):
         iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
 
 
-def try_remove(img):
-    try:
-        os.remove(img)
-    except OSError:
-        pass
-
-
 def transaction_action(action, **kwargs):
     return {
         'type': action,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dcdcd0387f..15704ca466 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -515,12 +515,15 @@ class FilePath:
         return False
 
 
+def try_remove(img):
+    try:
+        os.remove(img)
+    except OSError:
+        pass
+
 def file_path_remover():
     for path in reversed(file_path_remover.paths):
-        try:
-            os.remove(path)
-        except OSError:
-            pass
+        try_remove(path)
 
 
 def file_path(*names, base_dir=test_dir):
-- 
2.29.2

[PATCH v2 3/8] iotests/129: Remove test images in tearDown()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/129 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 0e13244d85..2fc65ada6a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -47,6 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
                              **params)
         self.vm.shutdown()
+        for img in (self.test_img, self.target_img, self.base_img):
+            iotests.try_remove(img)
 
     def do_test_stop(self, cmd, **args):
         """Test 'stop' while block job is running on a throttled drive.
-- 
2.29.2

[PATCH v2 4/8] iotests/129: Do not check @busy

[Max Reitz]

@busy is false when the job is paused, which happens all the time
because that is how jobs yield (e.g. for mirror at least since commit
565ac01f8d3).

Back when 129 was added (2015), perhaps there was no better way of
checking whether the job was still actually running.  Now we have the
@status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
that information.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/129 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 2fc65ada6a..dd23bb2e5a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         result = self.vm.qmp("stop")
         self.assert_qmp(result, 'return', {})
         result = self.vm.qmp("query-block-jobs")
-        self.assert_qmp(result, 'return[0]/busy', True)
+        self.assert_qmp(result, 'return[0]/status', 'running')
         self.assert_qmp(result, 'return[0]/ready', False)
 
     def test_drive_mirror(self):
-- 
2.29.2

[PATCH v2 5/8] iotests/129: Use throttle node

[Max Reitz]

Throttling on the BB has not affected block jobs in a while, so it is
possible that one of the jobs in 129 finishes before the VM is stopped.
We can fix that by running the job from a throttle node.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129 | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index dd23bb2e5a..58536bc6ee 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
                          "-b", self.base_img, '-F', iotests.imgfmt)
         iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
-        self.vm = iotests.VM().add_drive(self.test_img)
+        self.vm = iotests.VM()
+        self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
+
+        source_drive = 'driver=throttle,' \
+                       'throttle-group=tg0,' \
+                      f'file.driver={iotests.imgfmt},' \
+                      f'file.file.filename={self.test_img}'
+
+        self.vm.add_drive(None, source_drive)
         self.vm.launch()
 
     def tearDown(self):
-        params = {"device": "drive0",
-                  "bps": 0,
-                  "bps_rd": 0,
-                  "bps_wr": 0,
-                  "iops": 0,
-                  "iops_rd": 0,
-                  "iops_wr": 0,
-                 }
-        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
-                             **params)
         self.vm.shutdown()
         for img in (self.test_img, self.target_img, self.base_img):
             iotests.try_remove(img)
@@ -53,33 +51,24 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
     def do_test_stop(self, cmd, **args):
         """Test 'stop' while block job is running on a throttled drive.
         The 'stop' command shouldn't drain the job"""
-        params = {"device": "drive0",
-                  "bps": 1024,
-                  "bps_rd": 0,
-                  "bps_wr": 0,
-                  "iops": 0,
-                  "iops_rd": 0,
-                  "iops_wr": 0,
-                 }
-        result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
-                             **params)
-        self.assert_qmp(result, 'return', {})
         result = self.vm.qmp(cmd, **args)
         self.assert_qmp(result, 'return', {})
+
         result = self.vm.qmp("stop")
         self.assert_qmp(result, 'return', {})
         result = self.vm.qmp("query-block-jobs")
+
         self.assert_qmp(result, 'return[0]/status', 'running')
         self.assert_qmp(result, 'return[0]/ready', False)
 
     def test_drive_mirror(self):
         self.do_test_stop("drive-mirror", device="drive0",
-                          target=self.target_img,
+                          target=self.target_img, format=iotests.imgfmt,
                           sync="full")
 
     def test_drive_backup(self):
         self.do_test_stop("drive-backup", device="drive0",
-                          target=self.target_img,
+                          target=self.target_img, format=iotests.imgfmt,
                           sync="full")
 
     def test_block_commit(self):
-- 
2.29.2

[PATCH v2 6/8] iotests/129: Actually test a commit job

[Max Reitz]

Before this patch, test_block_commit() performs an active commit, which
under the hood is a mirror job.  If we want to test various different
block jobs, we should perhaps run an actual commit job instead.

Doing so requires adding an overlay above the source node before the
commit is done (and then specifying the source node as the top node for
the commit job).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/129 | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 58536bc6ee..7b4b6649f0 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -26,6 +26,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
     test_img = os.path.join(iotests.test_dir, 'test.img')
     target_img = os.path.join(iotests.test_dir, 'target.img')
     base_img = os.path.join(iotests.test_dir, 'base.img')
+    overlay_img = os.path.join(iotests.test_dir, 'overlay.img')
 
     def setUp(self):
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
@@ -36,6 +37,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
 
         source_drive = 'driver=throttle,' \
+                       'node-name=source,' \
                        'throttle-group=tg0,' \
                       f'file.driver={iotests.imgfmt},' \
                       f'file.file.filename={self.test_img}'
@@ -45,7 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 
     def tearDown(self):
         self.vm.shutdown()
-        for img in (self.test_img, self.target_img, self.base_img):
+        for img in (self.test_img, self.target_img, self.base_img,
+                    self.overlay_img):
             iotests.try_remove(img)
 
     def do_test_stop(self, cmd, **args):
@@ -72,7 +75,27 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
                           sync="full")
 
     def test_block_commit(self):
-        self.do_test_stop("block-commit", device="drive0")
+        # Add overlay above the source node so that we actually use a
+        # commit job instead of a mirror job
+
+        iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
+                         '1G')
+
+        result = self.vm.qmp('blockdev-add', **{
+                                 'node-name': 'overlay',
+                                 'driver': iotests.imgfmt,
+                                 'file': {
+                                     'driver': 'file',
+                                     'filename': self.overlay_img
+                                 }
+                             })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-snapshot',
+                             node='source', overlay='overlay')
+        self.assert_qmp(result, 'return', {})
+
+        self.do_test_stop('block-commit', device='drive0', top_node='source')
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=["qcow2"],
-- 
2.29.2

[PATCH v2 7/8] iotests/129: Limit mirror job's buffer size

[Max Reitz]

Issuing 'stop' on the VM drains all nodes.  If the mirror job has many
large requests in flight, this may lead to significant I/O that looks a
bit like 'stop' would make the job try to complete (which is what 129
should verify not to happen).

We can limit the I/O in flight by limiting the buffer size, so mirror
will make very little progress during the 'stop' drain.

(We do not need to do anything about commit, which has a buffer size of
512 kB by default; or backup, which goes cluster by cluster.  Once we
have asynchronous requests for backup, that will change, but then we can
fine-tune the backup job to only perform a single request on a very
small chunk, too.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/129 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 7b4b6649f0..6d21470cd7 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -67,7 +67,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
     def test_drive_mirror(self):
         self.do_test_stop("drive-mirror", device="drive0",
                           target=self.target_img, format=iotests.imgfmt,
-                          sync="full")
+                          sync="full", buf_size=65536)
 
     def test_drive_backup(self):
         self.do_test_stop("drive-backup", device="drive0",
-- 
2.29.2

[PATCH v2 8/8] iotests/129: Clean up pylint and mypy complaints

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/129     | 6 ++++--
 tests/qemu-iotests/297     | 2 +-
 tests/qemu-iotests/297.out | 1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 6d21470cd7..64578493c1 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -20,9 +20,10 @@
 
 import os
 import iotests
-import time
 
 class TestStopWithBlockJob(iotests.QMPTestCase):
+    assert iotests.test_dir is not None
+
     test_img = os.path.join(iotests.test_dir, 'test.img')
     target_img = os.path.join(iotests.test_dir, 'target.img')
     base_img = os.path.join(iotests.test_dir, 'base.img')
@@ -32,7 +33,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
         iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
                          "-b", self.base_img, '-F', iotests.imgfmt)
-        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
+        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
+                        self.test_img)
         self.vm = iotests.VM()
         self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
 
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b1a7d6d5e8..88f00415c8 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -32,7 +32,7 @@ fi
 
 # TODO: Empty this list!
 skip_files=(
-    030 040 041 044 045 055 056 057 065 093 096 118 124 129 132 136 139 147 148
+    030 040 041 044 045 055 056 057 065 093 096 118 124 132 136 139 147 148
     149 151 152 155 163 165 169 194 196 199 202 203 205 206 207 208 210 211 212
     213 216 218 219 222 224 228 234 235 236 237 238 240 242 245 246 248 255 256
     257 258 260 262 264 266 274 277 280 281 295 296 298 299 300 302 303 304 307
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
index c5ebbf6a17..92cae940c5 100644
--- a/tests/qemu-iotests/297.out
+++ b/tests/qemu-iotests/297.out
@@ -1,5 +1,6 @@
 QA output created by 297
 Files to be checked:
+  129
   209
   254
   283
-- 
2.29.2

Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Eric Blake]

On 1/13/21 11:57 AM, Max Reitz wrote:
> I.e., all Python files in the qemu-iotests/ directory.
> 
> Most files of course do not pass, so there is an extensive skip list for
> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
> 
> (Alternatively, we could have the opposite, i.e. an explicit list of
> files that we do want to check, but I think it is better to check files
> by default.)

Concur with the choice for default.

> 
> I decided to include the list of files checked in the reference output,
> so we do not accidentally lose coverage of anything.  That means adding
> new Python tests will require a change to 297.out, but that should not
> be a problem.

I'm okay with that.

> 
> On the other hand, I decided to hide mypy's "Success" lines from the
> reference output, because they do not add anything useful.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/297     | 66 ++++++++++++++++++++++++++++++++++----
>  tests/qemu-iotests/297.out |  6 +++-
>  2 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 5c5420712b..b1a7d6d5e8 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then
>      _notrun "mypy not found"
>  fi
>  
> -pylint-3 --score=n iotests.py
> +# TODO: Empty this list!

:)


> +file_list=()
> +for file in *; do
> +    # Check files with a .py extension or a Python shebang
> +    # (Unless they are in the skip_files list)
> +    if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') ||
> +                          (head -n 1 "$file" | grep -q '^#!.*python'))

Bash has an (obsolete) operator (()) (behaves like a mix of $(()) and
'if'); when nesting subshells, POSIX recommends inserting a space to
avoid inadvertent triggering of the alternate semantics of the operator.
 But why do you need nested subshells?  This is equivalent:

if [ -f "$file" ] && (echo  "$file" | grep -q '\.py$' ||
                      head -n 1 "$file" | grep -q '^#!.*python')

> +    then
> +        skip_file=false
> +        for skip in "${skip_files[@]}"; do

bashism, but iotests require bash, so fine.

> +            if [ "$skip" = "$file" ]; then
> +                skip_file=true
> +                break
> +            fi
> +        done
> +
> +        if ! $skip_file; then
> +            file_list+=("$file")

Likewise.

Whether or not you strip the extra (),
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 5/8] iotests/129: Use throttle node

[Eric Blake]

On 1/13/21 11:57 AM, Max Reitz wrote:
> Throttling on the BB has not affected block jobs in a while, so it is
> possible that one of the jobs in 129 finishes before the VM is stopped.
> We can fix that by running the job from a throttle node.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/129 | 37 +++++++++++++------------------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 8/8] iotests/129: Clean up pylint and mypy complaints

[Eric Blake]

On 1/13/21 11:57 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/129     | 6 ++++--
>  tests/qemu-iotests/297     | 2 +-
>  tests/qemu-iotests/297.out | 1 +
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>


> +++ b/tests/qemu-iotests/297
> @@ -32,7 +32,7 @@ fi
>  
>  # TODO: Empty this list!
>  skip_files=(
> -    030 040 041 044 045 055 056 057 065 093 096 118 124 129 132 136 139 147 148
> +    030 040 041 044 045 055 056 057 065 093 096 118 124 132 136 139 147 148
>      149 151 152 155 163 165 169 194 196 199 202 203 205 206 207 208 210 211 212
>      213 216 218 219 222 224 228 234 235 236 237 238 240 242 245 246 248 255 256

Ragged right end prior to justified lines looks odd, but it's cosmetic
and not fatal to the patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Vladimir Sementsov-Ogievskiy]

13.01.2021 20:57, Max Reitz wrote:
> I.e., all Python files in the qemu-iotests/ directory.
> 
> Most files of course do not pass, so there is an extensive skip list for
> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
> 
> (Alternatively, we could have the opposite, i.e. an explicit list of
> files that we do want to check, but I think it is better to check files
> by default.)
> 
> I decided to include the list of files checked in the reference output,
> so we do not accidentally lose coverage of anything.  That means adding
> new Python tests will require a change to 297.out, but that should not
> be a problem.

I have a parallel series, "Rework iotests/check", one of its aims is drop
group file, to avoid these endless conflicts in group file when you want
to send series or when you are porting patches to/from downstream.

And you are trying to add one another "group" file :) I don't like the idea.

Why should we loose accidentally the coverage? Logic is extremely simple:
all files except for the list.

> 
> On the other hand, I decided to hide mypy's "Success" lines from the
> reference output, because they do not add anything useful.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/297     | 66 ++++++++++++++++++++++++++++++++++----
>   tests/qemu-iotests/297.out |  6 +++-
>   2 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 5c5420712b..b1a7d6d5e8 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then
>       _notrun "mypy not found"
>   fi
>   
> -pylint-3 --score=n iotests.py
> +# TODO: Empty this list!
> +skip_files=(
> +    030 040 041 044 045 055 056 057 065 093 096 118 124 129 132 136 139 147 148
> +    149 151 152 155 163 165 169 194 196 199 202 203 205 206 207 208 210 211 212
> +    213 216 218 219 222 224 228 234 235 236 237 238 240 242 245 246 248 255 256
> +    257 258 260 262 264 266 274 277 280 281 295 296 298 299 300 302 303 304 307
> +    nbd-fault-injector.py qcow2.py qcow2_format.py qed.py
> +)
>   
> -MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
> -    --disallow-any-generics --disallow-incomplete-defs \
> -    --disallow-untyped-decorators --no-implicit-optional \
> -    --warn-redundant-casts --warn-unused-ignores \
> -    --no-implicit-reexport iotests.py
> +file_list=()
> +for file in *; do
> +    # Check files with a .py extension or a Python shebang
> +    # (Unless they are in the skip_files list)
> +    if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') ||
> +                          (head -n 1 "$file" | grep -q '^#!.*python'))
> +    then
> +        skip_file=false
> +        for skip in "${skip_files[@]}"; do
> +            if [ "$skip" = "$file" ]; then
> +                skip_file=true
> +                break
> +            fi
> +        done
> +
> +        if ! $skip_file; then
> +            file_list+=("$file")
> +        fi
> +    fi
> +done
> +
> +# Emit list of all files that are checked so we will not accidentally
> +# lose coverage
> +echo 'Files to be checked:'
> +
> +file_list_str=''
> +for file in "${file_list[@]}"; do
> +    echo "  $file"
> +done | sort
> +
> +# We can pass all files to pylint at once...
> +pylint-3 --score=n "${file_list[@]}"
> +
> +# ...but mypy needs to be called once per file.  Otherwise, it will
> +# interpret all given files as belonging together (i.e., they may not
> +# both define the same classes, etc.; most notably, they must not both
> +# define the __main__ module).
> +for file in "${file_list[@]}"; do
> +    mypy_output=$(
> +        MYPYPATH=../../python/ mypy --warn-unused-configs \
> +            --disallow-subclassing-any \
> +            --disallow-any-generics --disallow-incomplete-defs \
> +            --disallow-untyped-decorators --no-implicit-optional \
> +            --warn-redundant-casts --warn-unused-ignores \
> +            --no-implicit-reexport "$file" \
> +            2>&1
> +    )
> +
> +    if [ $? != 0 ]; then
> +        echo "$mypy_output"
> +    fi
> +done
>   
>   # success, all done
>   echo "*** done"
> diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
> index 6acc843649..c5ebbf6a17 100644
> --- a/tests/qemu-iotests/297.out
> +++ b/tests/qemu-iotests/297.out
> @@ -1,3 +1,7 @@
>   QA output created by 297
> -Success: no issues found in 1 source file
> +Files to be checked:
> +  209
> +  254
> +  283
> +  iotests.py
>   *** done
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/8] iotests: Move try_remove to iotests.py

[Vladimir Sementsov-Ogievskiy]

13.01.2021 20:57, Max Reitz wrote:
> Signed-off-by: Max Reitz<mreitz@redhat.com>
> Reviewed-by: Eric Blake<eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Vladimir Sementsov-Ogievskiy]

13.01.2021 22:27, Vladimir Sementsov-Ogievskiy wrote:
> 13.01.2021 20:57, Max Reitz wrote:
>> I.e., all Python files in the qemu-iotests/ directory.
>>
>> Most files of course do not pass, so there is an extensive skip list for
>> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
>>
>> (Alternatively, we could have the opposite, i.e. an explicit list of
>> files that we do want to check, but I think it is better to check files
>> by default.)
>>
>> I decided to include the list of files checked in the reference output,
>> so we do not accidentally lose coverage of anything.  That means adding
>> new Python tests will require a change to 297.out, but that should not
>> be a problem.
> 
> I have a parallel series, "Rework iotests/check", one of its aims is drop
> group file, to avoid these endless conflicts in group file when you want
> to send series or when you are porting patches to/from downstream.
> 
> And you are trying to add one another "group" file :) I don't like the idea.
> 
> Why should we loose accidentally the coverage? Logic is extremely simple:
> all files except for the list.
> 

Also.. What about checking python in python :) ? I exercised myself,
rewriting it into python. Take it if you like:
(suddenly, pylint warns about "TODO"s, so I just drop a TODO line.. Probably
  we'll have to suppress this warning in 297)



import os
import shutil
import subprocess

import iotests

iotests.script_initialize()


def is_python_file(filename):
     if filename.endswith('.py'):
         return True

     with open(filename) as f:
         try:
             first_line = f.readline()
             if first_line.startswith('#!') and 'python' in first_line:
                 return True
         except UnicodeDecodeError:  # ignore core files, etc
             pass

     return False


for linter in ('pylint-3', 'mypy'):
     if shutil.which(linter) is None:
         iotests.notrun(f'{linter} not found')

skip_files = (
     '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
     '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
     '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
     '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
     '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
     '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
     '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
     '299', '300', '302', '303', '304', '307', 'nbd-fault-injector.py',
     'qcow2.py', 'qcow2_format.py', 'qed.py'
)

files = [f for f in (set(os.listdir('.')) - set(skip_files))
          if os.path.isfile(f) and is_python_file(f)]

# We can pass all files to pylint at once...
subprocess.run(['pylint-3', '--score=n'] + files, check=False)

# ...but mypy needs to be called once per file.  Otherwise, it will
# interpret all given files as belonging together (i.e., they may not
# both define the same classes, etc.; most notably, they must not both
# define the __main__ module).
os.environ['MYPYPATH'] = '../../python/'
for file in files:
     p = subprocess.run(['mypy', '--warn-unused-configs',
                         '--disallow-subclassing-any',
                         '--disallow-any-generics',
                         '--disallow-incomplete-defs',
                         '--disallow-untyped-decorators',
                         '--no-implicit-optional', '--warn-redundant-casts',
                         '--warn-unused-ignores', '--no-implicit-reexport',
                         file], check=False,
                        stdout=subprocess.PIPE,
                        stderr=subprocess.STDOUT)

     if p.returncode != 0:
         print(p.stdout)




-- 
Best regards,
Vladimir

Re: [PATCH v2 5/8] iotests/129: Use throttle node

[Vladimir Sementsov-Ogievskiy]

13.01.2021 20:57, Max Reitz wrote:
> Throttling on the BB has not affected block jobs in a while, so it is
> possible that one of the jobs in 129 finishes before the VM is stopped.
> We can fix that by running the job from a throttle node.
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 8/8] iotests/129: Clean up pylint and mypy complaints

[Vladimir Sementsov-Ogievskiy]

13.01.2021 20:57, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/129     | 6 ++++--
>   tests/qemu-iotests/297     | 2 +-
>   tests/qemu-iotests/297.out | 1 +
>   3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 6d21470cd7..64578493c1 100755
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -20,9 +20,10 @@
>   
>   import os
>   import iotests
> -import time
>   
>   class TestStopWithBlockJob(iotests.QMPTestCase):
> +    assert iotests.test_dir is not None

Hmm. Will we have to add such assertions to all python tests to pass mypy ? That's not good if so..

Still, OK for now:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +
>       test_img = os.path.join(iotests.test_dir, 'test.img')
>       target_img = os.path.join(iotests.test_dir, 'target.img')
>       base_img = os.path.join(iotests.test_dir, 'base.img')
> @@ -32,7 +33,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>           iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
>           iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
>                            "-b", self.base_img, '-F', iotests.imgfmt)
> -        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
> +        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
> +                        self.test_img)
>           self.vm = iotests.VM()
>           self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
>   
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index b1a7d6d5e8..88f00415c8 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -32,7 +32,7 @@ fi
>   
>   # TODO: Empty this list!
>   skip_files=(
> -    030 040 041 044 045 055 056 057 065 093 096 118 124 129 132 136 139 147 148
> +    030 040 041 044 045 055 056 057 065 093 096 118 124 132 136 139 147 148
>       149 151 152 155 163 165 169 194 196 199 202 203 205 206 207 208 210 211 212
>       213 216 218 219 222 224 228 234 235 236 237 238 240 242 245 246 248 255 256
>       257 258 260 262 264 266 274 277 280 281 295 296 298 299 300 302 303 304 307
> diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
> index c5ebbf6a17..92cae940c5 100644
> --- a/tests/qemu-iotests/297.out
> +++ b/tests/qemu-iotests/297.out
> @@ -1,5 +1,6 @@
>   QA output created by 297
>   Files to be checked:
> +  129
>     209
>     254
>     283
> 

-- 
Best regards,
Vladimir

Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Max Reitz]

On 13.01.21 20:01, Eric Blake wrote:
> On 1/13/21 11:57 AM, Max Reitz wrote:
>> I.e., all Python files in the qemu-iotests/ directory.
>>
>> Most files of course do not pass, so there is an extensive skip list for
>> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
>>
>> (Alternatively, we could have the opposite, i.e. an explicit list of
>> files that we do want to check, but I think it is better to check files
>> by default.)
> 
> Concur with the choice for default.
> 
>>
>> I decided to include the list of files checked in the reference output,
>> so we do not accidentally lose coverage of anything.  That means adding
>> new Python tests will require a change to 297.out, but that should not
>> be a problem.
> 
> I'm okay with that.
> 
>>
>> On the other hand, I decided to hide mypy's "Success" lines from the
>> reference output, because they do not add anything useful.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/297     | 66 ++++++++++++++++++++++++++++++++++----
>>   tests/qemu-iotests/297.out |  6 +++-
>>   2 files changed, 65 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index 5c5420712b..b1a7d6d5e8 100755
>> --- a/tests/qemu-iotests/297
>> +++ b/tests/qemu-iotests/297
>> @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then
>>       _notrun "mypy not found"
>>   fi
>>   
>> -pylint-3 --score=n iotests.py
>> +# TODO: Empty this list!
> 
> :)
> 
> 
>> +file_list=()
>> +for file in *; do
>> +    # Check files with a .py extension or a Python shebang
>> +    # (Unless they are in the skip_files list)
>> +    if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') ||
>> +                          (head -n 1 "$file" | grep -q '^#!.*python'))
> 
> Bash has an (obsolete) operator (()) (behaves like a mix of $(()) and
> 'if'); when nesting subshells, POSIX recommends inserting a space to
> avoid inadvertent triggering of the alternate semantics of the operator.
>   But why do you need nested subshells?  This is equivalent:
> 
> if [ -f "$file" ] && (echo  "$file" | grep -q '\.py$' ||
>                        head -n 1 "$file" | grep -q '^#!.*python')

I just wasn’t sure of the order of pipe and ||.  Sure, I can change it 
(depending on whether or not I rewrite it in Python, as suggested by 
Vladimir).

>> +    then
>> +        skip_file=false
>> +        for skip in "${skip_files[@]}"; do
> 
> bashism, but iotests require bash, so fine.
> 
>> +            if [ "$skip" = "$file" ]; then
>> +                skip_file=true
>> +                break
>> +            fi
>> +        done
>> +
>> +        if ! $skip_file; then
>> +            file_list+=("$file")
> 
> Likewise.
> 
> Whether or not you strip the extra (),
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Max Reitz]

On 13.01.21 20:27, Vladimir Sementsov-Ogievskiy wrote:
> 13.01.2021 20:57, Max Reitz wrote:
>> I.e., all Python files in the qemu-iotests/ directory.
>>
>> Most files of course do not pass, so there is an extensive skip list for
>> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
>>
>> (Alternatively, we could have the opposite, i.e. an explicit list of
>> files that we do want to check, but I think it is better to check files
>> by default.)
>>
>> I decided to include the list of files checked in the reference output,
>> so we do not accidentally lose coverage of anything.  That means adding
>> new Python tests will require a change to 297.out, but that should not
>> be a problem.
> 
> I have a parallel series, "Rework iotests/check", one of its aims is drop
> group file, to avoid these endless conflicts in group file when you want
> to send series or when you are porting patches to/from downstream.
> 
> And you are trying to add one another "group" file :) I don't like the 
> idea.

I understand.

> Why should we loose accidentally the coverage? Logic is extremely simple:
> all files except for the list.

I hope so.  I just felt better having a reassurance that we indeed check 
everything we want to check.

But if it isn’t feasible to keep this list, I guess we just can’t.

Max

Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Max Reitz]

On 13.01.21 21:28, Vladimir Sementsov-Ogievskiy wrote:
> 13.01.2021 22:27, Vladimir Sementsov-Ogievskiy wrote:
>> 13.01.2021 20:57, Max Reitz wrote:
>>> I.e., all Python files in the qemu-iotests/ directory.
>>>
>>> Most files of course do not pass, so there is an extensive skip list for
>>> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
>>>
>>> (Alternatively, we could have the opposite, i.e. an explicit list of
>>> files that we do want to check, but I think it is better to check files
>>> by default.)
>>>
>>> I decided to include the list of files checked in the reference output,
>>> so we do not accidentally lose coverage of anything.  That means adding
>>> new Python tests will require a change to 297.out, but that should not
>>> be a problem.
>>
>> I have a parallel series, "Rework iotests/check", one of its aims is drop
>> group file, to avoid these endless conflicts in group file when you want
>> to send series or when you are porting patches to/from downstream.
>>
>> And you are trying to add one another "group" file :) I don't like the 
>> idea.
>>
>> Why should we loose accidentally the coverage? Logic is extremely simple:
>> all files except for the list.
>>
> 
> Also.. What about checking python in python :) ? I exercised myself,
> rewriting it into python. Take it if you like:

Why not, actually.

> (suddenly, pylint warns about "TODO"s, so I just drop a TODO line.. 
> Probably
>   we'll have to suppress this warning in 297)

I’d suppress this warning altogether, no?  I would like to keep TODO 
lines in other tests, too, without 297 failing.

Max

Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Vladimir Sementsov-Ogievskiy]

14.01.2021 12:31, Max Reitz wrote:
> On 13.01.21 21:28, Vladimir Sementsov-Ogievskiy wrote:
>> 13.01.2021 22:27, Vladimir Sementsov-Ogievskiy wrote:
>>> 13.01.2021 20:57, Max Reitz wrote:
>>>> I.e., all Python files in the qemu-iotests/ directory.
>>>>
>>>> Most files of course do not pass, so there is an extensive skip list for
>>>> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
>>>>
>>>> (Alternatively, we could have the opposite, i.e. an explicit list of
>>>> files that we do want to check, but I think it is better to check files
>>>> by default.)
>>>>
>>>> I decided to include the list of files checked in the reference output,
>>>> so we do not accidentally lose coverage of anything.  That means adding
>>>> new Python tests will require a change to 297.out, but that should not
>>>> be a problem.
>>>
>>> I have a parallel series, "Rework iotests/check", one of its aims is drop
>>> group file, to avoid these endless conflicts in group file when you want
>>> to send series or when you are porting patches to/from downstream.
>>>
>>> And you are trying to add one another "group" file :) I don't like the idea.
>>>
>>> Why should we loose accidentally the coverage? Logic is extremely simple:
>>> all files except for the list.
>>>
>>
>> Also.. What about checking python in python :) ? I exercised myself,
>> rewriting it into python. Take it if you like:
> 
> Why not, actually.
> 
>> (suddenly, pylint warns about "TODO"s, so I just drop a TODO line.. Probably
>>   we'll have to suppress this warning in 297)
> 
> I’d suppress this warning altogether, no?  I would like to keep TODO lines in other tests, too, without 297 failing.
> 

I'm for. Otherwise it means we just can't use "TODO" things)


-- 
Best regards,
Vladimir