diff mbox series

iotests: Drop deprecated 'props' from object-add

Message ID 20210216171653.6543-1-berto@igalia.com (mailing list archive)
State New, archived
Headers show
Series iotests: Drop deprecated 'props' from object-add | expand

Commit Message

Alberto Garcia Feb. 16, 2021, 5:16 p.m. UTC
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/087     |  8 ++------
 tests/qemu-iotests/184     | 18 ++++++------------
 tests/qemu-iotests/218     |  2 +-
 tests/qemu-iotests/235     |  2 +-
 tests/qemu-iotests/245     |  4 ++--
 tests/qemu-iotests/258     |  7 +++----
 tests/qemu-iotests/258.out |  4 ++--
 tests/qemu-iotests/295     |  2 +-
 tests/qemu-iotests/296     |  2 +-
 9 files changed, 19 insertions(+), 30 deletions(-)

Comments

Max Reitz Feb. 19, 2021, 12:04 p.m. UTC | #1
On 16.02.21 18:16, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   tests/qemu-iotests/087     |  8 ++------
>   tests/qemu-iotests/184     | 18 ++++++------------
>   tests/qemu-iotests/218     |  2 +-
>   tests/qemu-iotests/235     |  2 +-
>   tests/qemu-iotests/245     |  4 ++--
>   tests/qemu-iotests/258     |  7 +++----
>   tests/qemu-iotests/258.out |  4 ++--
>   tests/qemu-iotests/295     |  2 +-
>   tests/qemu-iotests/296     |  2 +-
>   9 files changed, 19 insertions(+), 30 deletions(-)

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

Two Python syntax nit picks below.

[...]

> diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
> index ae7c4fb187..cbb38923cf 100755
> --- a/tests/qemu-iotests/218
> +++ b/tests/qemu-iotests/218
> @@ -152,7 +152,7 @@ with iotests.VM() as vm, \
>       vm.launch()
>   
>       ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',
> -                 props={'x-bps-read': 4096})
> +                 x_bps_read = 4096)

To stay consistent, I think there shouldn’t be spaces around '=' here.

(flake8 thinks so, too)

>       assert ret['return'] == {}
>   
>       ret = vm.qmp('blockdev-add',

[..]

> diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
> index 9a2d33ae5e..65ce02501a 100755
> --- a/tests/qemu-iotests/258
> +++ b/tests/qemu-iotests/258
> @@ -103,10 +103,9 @@ def test_concurrent_finish(write_to_stream_node):
>           vm.qmp_log('object-add',
>                      qom_type='throttle-group',
>                      id='tg',
> -                   props={
> -                       'x-iops-write': 1,
> -                       'x-iops-write-max': 1
> -                   })
> +                   x_iops_write=1,
> +                   x_iops_write_max=1
> +                   )

This indentation looks weird to me now.  Unfortunately, flake8 finds 
this is the only correct indentation, so I have no reason to complain.

Perhaps putting it on the preceding line would be better?
Kevin Wolf Feb. 19, 2021, 12:21 p.m. UTC | #2
Am 16.02.2021 um 18:16 hat Alberto Garcia geschrieben:
> Signed-off-by: Alberto Garcia <berto@igalia.com>

> diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> index 20d16dbf38..f5c73b9c17 100755
> --- a/tests/qemu-iotests/235
> +++ b/tests/qemu-iotests/235
> @@ -57,7 +57,7 @@ vm.add_args('-drive', 'id=src,file=' + disk)
>  vm.launch()
>  
>  log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
> -           props={ 'x-bps-total': size }))
> +           x_bps_total=size))

x-bps-total isn't a stable interface, I'd prefer to use limits.

My patch from November [1] had this:

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index d1b10ac36b..2765561ada 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -56,7 +56,7 @@ vm.add_args('-drive', 'id=src,file=' + disk)
 vm.launch()

 log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
-           props={ 'x-bps-total': size }))
+           limits={ 'bps-total': size }))

 log(vm.qmp('blockdev-add',
            **{ 'node-name': 'target',

The same happens in other hunks in the patch.

Actually, I believe I can even merge my patch to drop 'props' while
the rest of the object-add QAPIfication series isn't ready yet. Maybe I
should just do that.

Kevin

[1] https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00802.html
Alberto Garcia Feb. 19, 2021, 12:35 p.m. UTC | #3
On Fri 19 Feb 2021 01:04:00 PM CET, Max Reitz <mreitz@redhat.com> wrote:
> Two Python syntax nit picks below.

>>       ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',
>> -                 props={'x-bps-read': 4096})
>> +                 x_bps_read = 4096)
>
> To stay consistent, I think there shouldn’t be spaces around '=' here.

Right, I didn't notice that.

>> @@ -103,10 +103,9 @@ def test_concurrent_finish(write_to_stream_node):
>>           vm.qmp_log('object-add',
>>                      qom_type='throttle-group',
>>                      id='tg',
>> -                   props={
>> -                       'x-iops-write': 1,
>> -                       'x-iops-write-max': 1
>> -                   })
>> +                   x_iops_write=1,
>> +                   x_iops_write_max=1
>> +                   )
>
> This indentation looks weird to me now.  Unfortunately, flake8 finds
> this is the only correct indentation, so I have no reason to complain.
>
> Perhaps putting it on the preceding line would be better?

I'm fine either way, I can resend the patch with Kevin's suggestions.

Berto
Alberto Garcia Feb. 19, 2021, 12:45 p.m. UTC | #4
On Fri 19 Feb 2021 01:21:49 PM CET, Kevin Wolf <kwolf@redhat.com> wrote:
>>  log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
>> -           props={ 'x-bps-total': size }))
>> +           x_bps_total=size))
>
> x-bps-total isn't a stable interface, I'd prefer to use limits.
>
> My patch from November [1] had this:

Do you want me to resend mine, or wait for yours, or what then? :)

Berto
Kevin Wolf Feb. 19, 2021, 1:26 p.m. UTC | #5
Am 19.02.2021 um 13:45 hat Alberto Garcia geschrieben:
> On Fri 19 Feb 2021 01:21:49 PM CET, Kevin Wolf <kwolf@redhat.com> wrote:
> >>  log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
> >> -           props={ 'x-bps-total': size }))
> >> +           x_bps_total=size))
> >
> > x-bps-total isn't a stable interface, I'd prefer to use limits.
> >
> > My patch from November [1] had this:
> 
> Do you want me to resend mine, or wait for yours, or what then? :)

It's your patch, you decide. :-)

I haven't compared the patches in detail yet, so if you think merging my
patch has the same result and is less work, I can do that. If not and
you send a v2, I'll take that. Or you see differences and post review
comments to my patch instead of a v2 of yours. I'm fine with any of
these options.

Kevin
diff mbox series

Patch

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index edd43f1a28..d8e0e384cd 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -143,9 +143,7 @@  run_qemu <<EOF
   "arguments": {
       "qom-type": "secret",
       "id": "sec0",
-      "props": {
-          "data": "123456"
-      }
+      "data": "123456"
   }
 }
 { "execute": "blockdev-add",
@@ -176,9 +174,7 @@  run_qemu <<EOF
   "arguments": {
       "qom-type": "secret",
       "id": "sec0",
-      "props": {
-          "data": "123456"
-      }
+      "data": "123456"
   }
 }
 { "execute": "blockdev-add",
diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
index 513d167098..e4cbcd8634 100755
--- a/tests/qemu-iotests/184
+++ b/tests/qemu-iotests/184
@@ -67,10 +67,8 @@  run_qemu <<EOF
   "arguments": {
     "qom-type": "throttle-group",
     "id": "group0",
-    "props": {
-      "limits" : {
-        "iops-total": 1000
-      }
+    "limits" : {
+      "iops-total": 1000
     }
   }
 }
@@ -96,10 +94,8 @@  run_qemu <<EOF
   "arguments": {
     "qom-type": "throttle-group",
     "id": "group0",
-    "props" : {
-      "limits": {
-          "iops-total": 1000
-      }
+    "limits": {
+        "iops-total": 1000
     }
   }
 }
@@ -136,10 +132,8 @@  run_qemu <<EOF
   "arguments": {
     "qom-type": "throttle-group",
     "id": "group0",
-    "props" : {
-      "limits": {
-          "iops-total": 1000
-      }
+    "limits": {
+        "iops-total": 1000
     }
   }
 }
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index ae7c4fb187..cbb38923cf 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -152,7 +152,7 @@  with iotests.VM() as vm, \
     vm.launch()
 
     ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',
-                 props={'x-bps-read': 4096})
+                 x_bps_read = 4096)
     assert ret['return'] == {}
 
     ret = vm.qmp('blockdev-add',
diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index 20d16dbf38..f5c73b9c17 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -57,7 +57,7 @@  vm.add_args('-drive', 'id=src,file=' + disk)
 vm.launch()
 
 log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
-           props={ 'x-bps-total': size }))
+           x_bps_total=size))
 
 log(vm.qmp('blockdev-add',
            **{ 'node-name': 'target',
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index cfdeb902be..30b1d7b22d 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -644,12 +644,12 @@  class TestBlockdevReopen(iotests.QMPTestCase):
         ###### throttle ######
         ######################
         opts = { 'qom-type': 'throttle-group', 'id': 'group0',
-                 'props': { 'limits': { 'iops-total': 1000 } } }
+                 'limits': { 'iops-total': 1000 } }
         result = self.vm.qmp('object-add', conv_keys = False, **opts)
         self.assert_qmp(result, 'return', {})
 
         opts = { 'qom-type': 'throttle-group', 'id': 'group1',
-                 'props': { 'limits': { 'iops-total': 2000 } } }
+                 'limits': { 'iops-total': 2000 } }
         result = self.vm.qmp('object-add', conv_keys = False, **opts)
         self.assert_qmp(result, 'return', {})
 
diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index 9a2d33ae5e..65ce02501a 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -103,10 +103,9 @@  def test_concurrent_finish(write_to_stream_node):
         vm.qmp_log('object-add',
                    qom_type='throttle-group',
                    id='tg',
-                   props={
-                       'x-iops-write': 1,
-                       'x-iops-write-max': 1
-                   })
+                   x_iops_write=1,
+                   x_iops_write_max=1
+                   )
 
         vm.qmp_log('blockdev-add',
                    filters=[filter_qmp_testfiles, filter_qmp_imgfmt],
diff --git a/tests/qemu-iotests/258.out b/tests/qemu-iotests/258.out
index ce6e9ba3e5..bfcaf97afe 100644
--- a/tests/qemu-iotests/258.out
+++ b/tests/qemu-iotests/258.out
@@ -2,7 +2,7 @@  Running tests:
 
 === Commit and stream finish concurrently (letting stream write) ===
 
-{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"execute": "object-add", "arguments": {"id": "tg", "qom-type": "throttle-group", "x-iops-write": 1, "x-iops-write-max": 1}}
 {"return": {}}
 {"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "throttle-group": "tg"}, "node-name": "node4"}}
 {"return": {}}
@@ -18,7 +18,7 @@  Running tests:
 
 === Commit and stream finish concurrently (letting commit write) ===
 
-{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"execute": "object-add", "arguments": {"id": "tg", "qom-type": "throttle-group", "x-iops-write": 1, "x-iops-write-max": 1}}
 {"return": {}}
 {"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "throttle-group": "tg"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "node-name": "node4"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/295 b/tests/qemu-iotests/295
index 01a6c0b31f..270ad3999f 100755
--- a/tests/qemu-iotests/295
+++ b/tests/qemu-iotests/295
@@ -43,7 +43,7 @@  class Secret:
 
     def to_qmp_object(self):
         return { "qom_type" : "secret", "id": self.id(),
-                 "props": { "data": self.secret() } }
+                 "data": self.secret() }
 
 ################################################################################
 class EncryptionSetupTestCase(iotests.QMPTestCase):
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index 0bc3c6c7d7..7c65e987a1 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -43,7 +43,7 @@  class Secret:
 
     def to_qmp_object(self):
         return { "qom_type" : "secret", "id": self.id(),
-                 "props": { "data": self.secret() } }
+                 "data": self.secret() }
 
 ################################################################################