| Message ID | 20210216171653.6543-1-berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: Drop deprecated 'props' from object-add | expand |
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?
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
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
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
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 --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() } ################################################################################
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(-)