[PATCH v2 01/10] tests/qemu-iotests: hotfix for 307, 223 output
Fixes: 58a6fdcc Signed-off-by: John Snow Tested-by: Daniel P. Berrangé Reviewed-by: Daniel P. Berrangé --- tests/qemu-iotests/223.out | 4 ++-- tests/qemu-iotests/307.out | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index 06479415312..26fb347c5da 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -93,7 +93,7 @@ exports available: 3 export: 'n2' description: some text size: 4194304 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: 1 opt block: 4096 max block: 33554432 @@ -212,7 +212,7 @@ exports available: 3 export: 'n2' description: some text size: 4194304 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: 1 opt block: 4096 max block: 33554432 diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out index ec8d2be0e0a..390f05d1b78 100644 --- a/tests/qemu-iotests/307.out +++ b/tests/qemu-iotests/307.out @@ -83,7 +83,7 @@ exports available: 2 export: 'export1' description: This is the writable second export size: 67108864 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: XXX opt block: XXX max block: XXX @@ -109,7 +109,7 @@ exports available: 1 export: 'export1' description: This is the writable second export size: 67108864 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: XXX opt block: XXX max block: XXX -- 2.34.3
[PATCH v2 04/10] tests/vm: use 'cp' instead of 'ln' for temporary vm images
If the initial setup fails, you've permanently altered the state of the
downloaded image in an unknowable way. Use 'cp' like our other test
setup scripts do.
Signed-off-by: John Snow
Reviewed-by: Thomas Huth
---
tests/vm/centos | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/vm/centos b/tests/vm/centos
index 5c7bc1c1a9a..be4f6ff2f14 100755
--- a/tests/vm/centos
+++ b/tests/vm/centos
@@ -34,7 +34,7 @@ class CentosVM(basevm.BaseVM):
def build_image(self, img):
cimg =
self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2";)
img_tmp = img + ".tmp"
-subprocess.check_call(["ln", "-f", cimg, img_tmp])
+subprocess.check_call(['cp', '-f', cimg, img_tmp])
self.exec_qemu_img("resize", img_tmp, "50G")
self.boot(img_tmp, extra_args = ["-cdrom", self.gen_cloud_init_iso()])
self.wait_ssh()
--
2.34.3
Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded
On Wed, Jun 15, 2022 at 11:33 AM Daniel P. Berrangé wrote: > > On Wed, Jun 15, 2022 at 09:41:32AM -0400, John Snow wrote: > > On Tue, Jun 14, 2022 at 10:30 AM John Snow wrote: > > > > > > On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé > > > wrote: > > > > > > > > On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote: > > > > > On 14/06/2022 03.50, John Snow wrote: > > > > > > In certain container environments we may not have FUSE at all, so > > > > > > skip > > > > > > the test in this circumstance too. > > > > > > > > > > > > Signed-off-by: John Snow > > > > > > --- > > > > > > tests/qemu-iotests/108 | 6 ++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108 > > > > > > index 9e923d6a59f..e401c5e9933 100755 > > > > > > --- a/tests/qemu-iotests/108 > > > > > > +++ b/tests/qemu-iotests/108 > > > > > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then > > > > > > else > > > > > > loopdev=false > > > > > > +# Check for fuse support in the host environment: > > > > > > +lsmod | grep fuse &>/dev/null; > > > > > > > > > > That doesn't work if fuse has been linked statically into the kernel. > > > > > Would > > > > > it make sense to test for /sys/fs/fuse instead? > > > > > > > > > > (OTOH, we likely hardly won't run this on statically linked kernels > > > > > anyway, > > > > > so it might not matter too much) > > > > > > > > But more importantly 'lsmod' may not be installed in our container > > > > images. So checking /sys/fs/fuse avoids introducing a dep on the > > > > 'kmod' package. > > > > > > > > > > > > > > > +if [[ $? -ne 0 ]]; then > > > > > > > > > > I'd prefer single "[" instead of "[[" ... but since we're requiring > > > > > bash > > > > > anyway, it likely doesn't matter. > > > > > > > > Or > > > > > > > > if test $? != 0 ; then > > > > > > > > > > > > > > > +_notrun 'No Passwordless sudo nor FUSE kernel module' > > > > > > +fi > > > > > > + > > > > > > # QSD --export fuse will either yield "Parameter 'id' is > > > > > > missing" > > > > > > # or "Invalid parameter 'fuse'", depending on whether there is > > > > > > # FUSE support or not. > > > > > > > > > > > Good suggestions, thanks! > > > > > > > I think I need to test against /dev/fuse instead, because /sys/fs/fuse > > actually exists, but because of docker permissions (etc), FUSE isn't > > actually usable from the child container. > > > > I wound up with this: > > > > # Check for usable FUSE in the host environment: > > if test ! -c "/dev/fuse"; then > > _notrun 'No passwordless sudo nor usable /dev/fuse' > > fi > > > > Seems to work for my case here, at least, but I don't have a good > > sense for how broadly flexible it might be. It might be nicer to > > concoct some kind of NOP fuse mount instead, but I wasn't able to > > figure out such a command quickly. > > > > The next problem I have is actually related; test-qga (for the > > Centos.x86_64 run) is failing because the guest agent is reading > > /proc/self/mountinfo -- which contains entries for block devices that > > are not visible in the current container scope. I think when QGA goes > > to read info about these devices to populate a response, it chokes. > > This might be a genuine bug in QGA if we want it to tolerate existing > > inside of a container. > > Yes, we should fix this. Even if you don't run QGA in a container, > someone might configure the systemd service to harden it, by > restricting what /dev it is able to see and thus trigger the > same issue. Naive solution: if we try to look in /sys/dev/block/%u:%u and find that we are unable to do so for whatever reason (ENOENT et al), just skip that entry for the fsinfo returned to the caller. Does it need to be fancier than that? --js
Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded
On Tue, Jun 14, 2022 at 10:30 AM John Snow wrote: > > On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé > wrote: > > > > On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote: > > > On 14/06/2022 03.50, John Snow wrote: > > > > In certain container environments we may not have FUSE at all, so skip > > > > the test in this circumstance too. > > > > > > > > Signed-off-by: John Snow > > > > --- > > > > tests/qemu-iotests/108 | 6 ++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108 > > > > index 9e923d6a59f..e401c5e9933 100755 > > > > --- a/tests/qemu-iotests/108 > > > > +++ b/tests/qemu-iotests/108 > > > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then > > > > else > > > > loopdev=false > > > > +# Check for fuse support in the host environment: > > > > +lsmod | grep fuse &>/dev/null; > > > > > > That doesn't work if fuse has been linked statically into the kernel. > > > Would > > > it make sense to test for /sys/fs/fuse instead? > > > > > > (OTOH, we likely hardly won't run this on statically linked kernels > > > anyway, > > > so it might not matter too much) > > > > But more importantly 'lsmod' may not be installed in our container > > images. So checking /sys/fs/fuse avoids introducing a dep on the > > 'kmod' package. > > > > > > > > > +if [[ $? -ne 0 ]]; then > > > > > > I'd prefer single "[" instead of "[[" ... but since we're requiring bash > > > anyway, it likely doesn't matter. > > > > Or > > > > if test $? != 0 ; then > > > > > > > > > +_notrun 'No Passwordless sudo nor FUSE kernel module' > > > > +fi > > > > + > > > > # QSD --export fuse will either yield "Parameter 'id' is missing" > > > > # or "Invalid parameter 'fuse'", depending on whether there is > > > > # FUSE support or not. > > > > > Good suggestions, thanks! > I think I need to test against /dev/fuse instead, because /sys/fs/fuse actually exists, but because of docker permissions (etc), FUSE isn't actually usable from the child container. I wound up with this: # Check for usable FUSE in the host environment: if test ! -c "/dev/fuse"; then _notrun 'No passwordless sudo nor usable /dev/fuse' fi Seems to work for my case here, at least, but I don't have a good sense for how broadly flexible it might be. It might be nicer to concoct some kind of NOP fuse mount instead, but I wasn't able to figure out such a command quickly. The next problem I have is actually related; test-qga (for the Centos.x86_64 run) is failing because the guest agent is reading /proc/self/mountinfo -- which contains entries for block devices that are not visible in the current container scope. I think when QGA goes to read info about these devices to populate a response, it chokes. This might be a genuine bug in QGA if we want it to tolerate existing inside of a container. --js
Re: [PATCH 4/5] tests/vm: switch CentOS 8 to CentOS 8 Stream
On Tue, Jun 14, 2022 at 5:09 AM Daniel P. Berrangé wrote: > > On Mon, Jun 13, 2022 at 09:50:43PM -0400, John Snow wrote: > > The old CentOS image didn't work anymore because it was already EOL at > > the beginning of 2022. > > > > Signed-off-by: John Snow > > --- > > tests/vm/centos | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/tests/vm/centos b/tests/vm/centos > > index be4f6ff2f14..f5bbdecf62d 100755 > > --- a/tests/vm/centos > > +++ b/tests/vm/centos > > @@ -1,8 +1,8 @@ > > #!/usr/bin/env python3 > > # > > -# CentOS image > > +# CentOS 8 Stream image > > # > > -# Copyright 2018 Red Hat Inc. > > +# Copyright 2018, 2022 Red Hat Inc. > > # > > # Authors: > > # Fam Zheng > > @@ -18,7 +18,7 @@ import basevm > > import time > > > > class CentosVM(basevm.BaseVM): > > -name = "centos" > > +name = "centos8s" > > > What's the effect of this ? It feels a little odd to set name to 'centos8s' > here but have this file still called just 'centos' - I assume the 'name' > variable was intended to always match the filename > Changes the logfile names in ~/.cache/qemu-vm, changes the hostname config in gen_cloud_init_iso(), not much else. You're right, though, I shouldn't change it in one place but not the other ... I'll just leave it as "centos". I felt compelled briefly to indicate it was "the newer, different CentOS" but with the old one being EOL I suppose it's easy enough to infer. --js
Re: [PATCH 3/5] tests/vm: use 'cp' instead of 'ln' for temporary vm images
On Tue, Jun 14, 2022 at 12:40 AM Thomas Huth wrote:
>
> On 14/06/2022 03.50, John Snow wrote:
> > If the initial setup fails, you've permanently altered the state of the
> > downloaded image in an unknowable way. Use 'cp' like our other test
> > setup scripts do.
> >
> > Signed-off-by: John Snow
> > ---
> > tests/vm/centos | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/vm/centos b/tests/vm/centos
> > index 5c7bc1c1a9a..be4f6ff2f14 100755
> > --- a/tests/vm/centos
> > +++ b/tests/vm/centos
> > @@ -34,7 +34,7 @@ class CentosVM(basevm.BaseVM):
> > def build_image(self, img):
> > cimg =
> > self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2";)
> > img_tmp = img + ".tmp"
> > -subprocess.check_call(["ln", "-f", cimg, img_tmp])
> > +subprocess.check_call(['cp', '-f', cimg, img_tmp])
>
> I wonder whether it would make sense to use "qemu-img create -b" instead to
> save some disk space?
>
> Anyway, your patch is certainly already an improvement, so:
>
> Reviewed-by: Thomas Huth
I wondered the same, but decided to keep a smaller series this time
around. VM tests already use a lot of space, so I doubt this is adding
new constraints that didn't exist before. A more rigorous overhaul may
be in order, but not right now. (It looks like the config file stuff
to override defaults is not necessarily rigorously respected by the
different installer recipes.)
I think the caching of the fully set-up image needs work, too. In
practice we leave the image sitting around, but we seem to always
rebuild it no matter what, so it's not that useful. There's a few
things that can be done here to drastically speed up some things,
but... later.
--js
Re: [PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded
On Tue, Jun 14, 2022 at 4:59 AM Daniel P. Berrangé wrote: > > On Tue, Jun 14, 2022 at 06:46:35AM +0200, Thomas Huth wrote: > > On 14/06/2022 03.50, John Snow wrote: > > > In certain container environments we may not have FUSE at all, so skip > > > the test in this circumstance too. > > > > > > Signed-off-by: John Snow > > > --- > > > tests/qemu-iotests/108 | 6 ++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108 > > > index 9e923d6a59f..e401c5e9933 100755 > > > --- a/tests/qemu-iotests/108 > > > +++ b/tests/qemu-iotests/108 > > > @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then > > > else > > > loopdev=false > > > +# Check for fuse support in the host environment: > > > +lsmod | grep fuse &>/dev/null; > > > > That doesn't work if fuse has been linked statically into the kernel. Would > > it make sense to test for /sys/fs/fuse instead? > > > > (OTOH, we likely hardly won't run this on statically linked kernels anyway, > > so it might not matter too much) > > But more importantly 'lsmod' may not be installed in our container > images. So checking /sys/fs/fuse avoids introducing a dep on the > 'kmod' package. > > > > > > +if [[ $? -ne 0 ]]; then > > > > I'd prefer single "[" instead of "[[" ... but since we're requiring bash > > anyway, it likely doesn't matter. > > Or > > if test $? != 0 ; then > > > > > > +_notrun 'No Passwordless sudo nor FUSE kernel module' > > > +fi > > > + > > > # QSD --export fuse will either yield "Parameter 'id' is missing" > > > # or "Invalid parameter 'fuse'", depending on whether there is > > > # FUSE support or not. > > Good suggestions, thanks! --js
[PATCH 3/5] tests/vm: use 'cp' instead of 'ln' for temporary vm images
If the initial setup fails, you've permanently altered the state of the
downloaded image in an unknowable way. Use 'cp' like our other test
setup scripts do.
Signed-off-by: John Snow
---
tests/vm/centos | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/vm/centos b/tests/vm/centos
index 5c7bc1c1a9a..be4f6ff2f14 100755
--- a/tests/vm/centos
+++ b/tests/vm/centos
@@ -34,7 +34,7 @@ class CentosVM(basevm.BaseVM):
def build_image(self, img):
cimg =
self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2";)
img_tmp = img + ".tmp"
-subprocess.check_call(["ln", "-f", cimg, img_tmp])
+subprocess.check_call(['cp', '-f', cimg, img_tmp])
self.exec_qemu_img("resize", img_tmp, "50G")
self.boot(img_tmp, extra_args = ["-cdrom", self.gen_cloud_init_iso()])
self.wait_ssh()
--
2.34.3
[PATCH 2/5] tests/qemu-iotests: skip 108 when FUSE is not loaded
In certain container environments we may not have FUSE at all, so skip the test in this circumstance too. Signed-off-by: John Snow --- tests/qemu-iotests/108 | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108 index 9e923d6a59f..e401c5e9933 100755 --- a/tests/qemu-iotests/108 +++ b/tests/qemu-iotests/108 @@ -60,6 +60,12 @@ if sudo -n losetup &>/dev/null; then else loopdev=false +# Check for fuse support in the host environment: +lsmod | grep fuse &>/dev/null; +if [[ $? -ne 0 ]]; then +_notrun 'No Passwordless sudo nor FUSE kernel module' +fi + # QSD --export fuse will either yield "Parameter 'id' is missing" # or "Invalid parameter 'fuse'", depending on whether there is # FUSE support or not. -- 2.34.3
[PATCH 4/5] tests/vm: switch CentOS 8 to CentOS 8 Stream
The old CentOS image didn't work anymore because it was already EOL at
the beginning of 2022.
Signed-off-by: John Snow
---
tests/vm/centos | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/vm/centos b/tests/vm/centos
index be4f6ff2f14..f5bbdecf62d 100755
--- a/tests/vm/centos
+++ b/tests/vm/centos
@@ -1,8 +1,8 @@
#!/usr/bin/env python3
#
-# CentOS image
+# CentOS 8 Stream image
#
-# Copyright 2018 Red Hat Inc.
+# Copyright 2018, 2022 Red Hat Inc.
#
# Authors:
# Fam Zheng
@@ -18,7 +18,7 @@ import basevm
import time
class CentosVM(basevm.BaseVM):
-name = "centos"
+name = "centos8s"
arch = "x86_64"
BUILD_SCRIPT = """
set -e;
@@ -32,7 +32,7 @@ class CentosVM(basevm.BaseVM):
"""
def build_image(self, img):
-cimg =
self._download_with_cache("https://cloud.centos.org/centos/8/x86_64/images/CentOS-8-GenericCloud-8.3.2011-20201204.2.x86_64.qcow2";)
+cimg =
self._download_with_cache("https://cloud.centos.org/centos/8-stream/x86_64/images/CentOS-Stream-GenericCloud-8-20220125.1.x86_64.qcow2";)
img_tmp = img + ".tmp"
subprocess.check_call(['cp', '-f', cimg, img_tmp])
self.exec_qemu_img("resize", img_tmp, "50G")
--
2.34.3
[PATCH 0/5] Update CentOS VM tests
37, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET},
{ "start": 65537, "length": 511, "depth": 0, "present": true, "zero": true,
"data": false, "offset": OFFSET}]
*** done
--- /tmp/qemu-test/src/tests/qemu-iotests/253.out
+++ /tmp/qemu-test/253.out.bad
@@ -3,16 +3,10 @@
=== Check mapping of unaligned raw image ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048575
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET},
-{ "start": 4096, "length": 1044480, "depth": 0, "present": true, "zero": true,
"data": false, "offset": OFFSET}]
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET},
-{ "start": 4096, "length": 1044480, "depth": 0, "present": true, "zero": true,
"data": false, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET}]
wrote 65535/65535 bytes at offset 983040
63.999 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET},
-{ "start": 4096, "length": 978944, "depth": 0, "present": true, "zero": true,
"data": false, "offset": OFFSET},
-{ "start": 983040, "length": 65536, "depth": 0, "present": true, "zero":
false, "data": true, "offset": OFFSET}]
-[{ "start": 0, "length": 4096, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET},
-{ "start": 4096, "length": 978944, "depth": 0, "present": true, "zero": true,
"data": false, "offset": OFFSET},
-{ "start": 983040, "length": 65536, "depth": 0, "present": true, "zero":
false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero": false,
"data": true, "offset": OFFSET}]
*** done
... I'll work my way through trying to understand all of these failures,
but any help would be appreciated to get these tests humming again.
Meanwhile, VM tests that I have observed to be in non-working condition
on latest upstream:
- centos (x86_64) - CentOS 8 is EOL
- centos.aarch64 - CentOS 8 is EOL, image is MIA
- haiku.x86_64 - build issues
- openbsd - virtio-net-failover tests hang indefinitely
- ubuntu.aarch64 - Honestly, I don't recall. I'm re-running overnight to
find out.
I'll continue to try and sort out the issues with all of the tests I am
seeing fail and gather better diagnostics and intel for each.
:(
--js
John Snow (5):
tests/qemu-iotests: hotfix for 307, 223 output
tests/qemu-iotests: skip 108 when FUSE is not loaded
tests/vm: use 'cp' instead of 'ln' for temporary vm images
tests/vm: switch CentOS 8 to CentOS 8 Stream
tests/vm: switch centos.aarch64 to CentOS 8 Stream
tests/qemu-iotests/108 | 6 ++
tests/qemu-iotests/223.out | 4 +-
tests/qemu-iotests/307.out | 4 +-
tests/vm/centos| 10 +--
tests/vm/centos.aarch64| 174 ++---
5 files changed, 41 insertions(+), 157 deletions(-)
--
2.34.3
[PATCH 1/5] tests/qemu-iotests: hotfix for 307, 223 output
Fixes: 58a6fdcc Signed-off-by: John Snow --- tests/qemu-iotests/223.out | 4 ++-- tests/qemu-iotests/307.out | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index 06479415312..26fb347c5da 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -93,7 +93,7 @@ exports available: 3 export: 'n2' description: some text size: 4194304 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: 1 opt block: 4096 max block: 33554432 @@ -212,7 +212,7 @@ exports available: 3 export: 'n2' description: some text size: 4194304 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: 1 opt block: 4096 max block: 33554432 diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out index ec8d2be0e0a..390f05d1b78 100644 --- a/tests/qemu-iotests/307.out +++ b/tests/qemu-iotests/307.out @@ -83,7 +83,7 @@ exports available: 2 export: 'export1' description: This is the writable second export size: 67108864 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: XXX opt block: XXX max block: XXX @@ -109,7 +109,7 @@ exports available: 1 export: 'export1' description: This is the writable second export size: 67108864 - flags: 0xced ( flush fua trim zeroes df cache fast-zero ) + flags: 0xded ( flush fua trim zeroes df multi cache fast-zero ) min block: XXX opt block: XXX max block: XXX -- 2.34.3
[RFC PATCH v2 1/7] tests: create optional tests/venv dependency groups
This patch uses a dummy package and setup.cfg/setup.py files to manage optional dependency groups for the test venv specification. Now, there's a core set of dependencies which for now includes just "qemu" (but soon, qemu.qmp) and a separate, optional 'avocado' group that includes avocado-framework and pycdlib. Practical upshot: We install only a minimum of things for the majority of check-* targets, but allow optional add-ons to be processed when running avocado tests. This will spare downstreams from having to add more dependencies than is necessary as a build dependencies when invoking "make check". (We also keep both sets of dependencies in one file, which is helpful for review to ensure that different option groups don't conflict with one another.) NOTE: There is a non-fatal caveat introduced by this patch on Ubuntu 20.04 systems; see the subsequent commit "tests: Remove spurious pip warnings on Ubuntu20.04" for more information. Signed-off-by: John Snow --- tests/Makefile.include | 21 ++--- tests/requirements.txt | 6 -- tests/setup.cfg| 20 tests/setup.py | 16 4 files changed, 50 insertions(+), 13 deletions(-) delete mode 100644 tests/requirements.txt create mode 100644 tests/setup.cfg create mode 100644 tests/setup.py diff --git a/tests/Makefile.include b/tests/Makefile.include index 3accb83b132..82c697230e0 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -81,13 +81,13 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES) # Python venv for running tests -.PHONY: check-venv check-avocado check-acceptance check-acceptance-deprecated-warning +.PHONY: check-venv check-venv-avocado check-avocado check-acceptance \ +check-acceptance-deprecated-warning # Build up our target list from the filtered list of ninja targets TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets))) TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv -TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python3 ifndef AVOCADO_TESTS @@ -108,10 +108,16 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \ $(TESTS_PYTHON) -m pip -q --disable-pip-version-check $1, \ "VENVPIP","$1") -$(TESTS_VENV_DIR): $(TESTS_VENV_REQ) +# Core dependencies for tests/venv +$(TESTS_VENV_DIR): $(SRC_PATH)/tests/setup.cfg $(SRC_PATH)/python/setup.cfg $(call quiet-command, $(PYTHON) -m venv $@, VENV, $@) $(call quiet-venv-pip,install -e "$(SRC_PATH)/python/") - $(call quiet-venv-pip,install -r $(TESTS_VENV_REQ)) + $(call quiet-venv-pip,install "$(SRC_PATH)/tests/") + $(call quiet-command, touch $@) + +# Optional avocado dependencies for tests/venv +$(TESTS_VENV_DIR)/avocado: $(TESTS_VENV_DIR) + $(call quiet-venv-pip,install "$(SRC_PATH)/tests/[avocado]") $(call quiet-command, touch $@) $(TESTS_RESULTS_DIR): @@ -119,6 +125,7 @@ $(TESTS_RESULTS_DIR): MKDIR, $@) check-venv: $(TESTS_VENV_DIR) +check-venv-avocado: $(TESTS_VENV_DIR)/avocado FEDORA_31_ARCHES_TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGETS))) FEDORA_31_ARCHES_CANDIDATES=$(patsubst ppc64,ppc64le,$(FEDORA_31_ARCHES_TARGETS)) @@ -126,16 +133,16 @@ FEDORA_31_ARCHES := x86_64 aarch64 ppc64le s390x FEDORA_31_DOWNLOAD=$(filter $(FEDORA_31_ARCHES),$(FEDORA_31_ARCHES_CANDIDATES)) # download one specific Fedora 31 image -get-vm-image-fedora-31-%: check-venv +get-vm-image-fedora-31-%: check-venv-avocado $(call quiet-command, \ $(TESTS_PYTHON) -m avocado vmimage get \ --distro=fedora --distro-version=31 --arch=$*, \ "AVOCADO", "Downloading avocado tests VM image for $*") # download all vm images, according to defined targets -get-vm-images: check-venv $(patsubst %,get-vm-image-fedora-31-%, $(FEDORA_31_DOWNLOAD)) +get-vm-images: check-venv-avocado $(patsubst %,get-vm-image-fedora-31-%, $(FEDORA_31_DOWNLOAD)) -check-avocado: check-venv $(TESTS_RESULTS_DIR) get-vm-images +check-avocado: check-venv-avocado $(TESTS_RESULTS_DIR) get-vm-images $(call quiet-command, \ $(TESTS_PYTHON) -m avocado \ --show=$(AVOCADO_SHOW) run --job-results-dir=$(TESTS_RESULTS_DIR) \ diff --git a/tests/requirements.txt b/tests/requirements.txt deleted file mode 100644 index 0ba561b6bdf..000 --- a/tests/requirements.txt +++ /dev/null @@ -1,6 +0,0 @@ -# Add Python module requirements, one per line, to be installed -# in the tests/venv Python virtual environment. For more info, -# refer to: https://pip.pypa.io/en/stable/user_guide/#id1 -# Note that qemu.git/python/ is always implicitly installed. -avocado-framework==88.1 -pycdlib==1.11.0 diff --git a/tests/setup.cfg b/tests/setup.cfg new file mode 100644 index 000..2
[RFC PATCH v2 2/7] tests: pythonize test venv creation
This splits the venv creation logic out of the Makefile and into a
Python script.
One reason for doing this is to be able to call the venv bootstrapper
from places outside of the Makefile, e.g. configure and iotests. Another
reason is to be able to add "offline" logic to modify the behavior of
the script in certain circumstances.
RFC: I don't like how I have an entire "enter_venv" function here, and
by the end of the series, completely separate logic for entering a venv
in testenv.py -- but they both operate in different ways: this patch
offers a method that alters the current ENV immediately, whereas the
testenv.py method alters a copied ENV that is explicitly passed to
subprocesses.
Still, there's a bit more boilerplate than I'd like, but it's probably
fine and it probably won't need to be updated much, but... less code is
usually better.
But, even if I did write it more generically here; iotests wouldn't be
able to use it anyway, because importing it would mean more sys.path
hacking. So... eh?
Signed-off-by: John Snow
---
tests/Makefile.include | 16 ++--
tests/mkvenv.py| 186 +
2 files changed, 192 insertions(+), 10 deletions(-)
create mode 100644 tests/mkvenv.py
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 82c697230e0..d8af6a38112 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -104,21 +104,17 @@ else
AVOCADO_CMDLINE_TAGS=$(addprefix -t , $(AVOCADO_TAGS))
endif
-quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
-$(TESTS_PYTHON) -m pip -q --disable-pip-version-check $1, \
-"VENVPIP","$1")
-
# Core dependencies for tests/venv
$(TESTS_VENV_DIR): $(SRC_PATH)/tests/setup.cfg $(SRC_PATH)/python/setup.cfg
- $(call quiet-command, $(PYTHON) -m venv $@, VENV, $@)
- $(call quiet-venv-pip,install -e "$(SRC_PATH)/python/")
- $(call quiet-venv-pip,install "$(SRC_PATH)/tests/")
- $(call quiet-command, touch $@)
+ $(call quiet-command, \
+$(PYTHON) "$(SRC_PATH)/tests/mkvenv.py" "$@", \
+MKVENV, $@)
# Optional avocado dependencies for tests/venv
$(TESTS_VENV_DIR)/avocado: $(TESTS_VENV_DIR)
- $(call quiet-venv-pip,install "$(SRC_PATH)/tests/[avocado]")
- $(call quiet-command, touch $@)
+ $(call quiet-command, \
+$(PYTHON) "$(SRC_PATH)/tests/mkvenv.py" --opt avocado "$<", \
+MKVENV, $@)
$(TESTS_RESULTS_DIR):
$(call quiet-command, mkdir -p $@, \
diff --git a/tests/mkvenv.py b/tests/mkvenv.py
new file mode 100644
index 000..0667106d6aa
--- /dev/null
+++ b/tests/mkvenv.py
@@ -0,0 +1,186 @@
+"""
+mkvenv - QEMU venv bootstrapping utility
+
+usage: mkvenv.py [-h] [--offline] [--opt OPT] target
+
+Bootstrap QEMU testing venv.
+
+positional arguments:
+ target Target directory to install virtual environment into.
+
+optional arguments:
+ -h, --help show this help message and exit
+ --offline Use system packages and work entirely offline.
+ --opt OPT Install an optional dependency group.
+"""
+
+# Copyright (C) 2022 Red Hat, Inc.
+#
+# Authors:
+# John Snow
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later. See the COPYING file in the top-level directory.
+
+# Do not add any dependencies from outside the stdlib:
+# This script must be usable on its own!
+
+import argparse
+from contextlib import contextmanager
+import logging
+import os
+from pathlib import Path
+import subprocess
+import sys
+from typing import Iterable, Iterator, Union
+import venv
+
+
+def make_venv(
+venv_path: Union[str, Path],
+system_site_packages: bool = False
+) -> None:
+"""
+Create a venv using the python3 'venv' module.
+
+Identical to:
+``python3 -m venv --clear [--system-site-packages] {venv_path}``
+
+:param venv_path: The target directory
+:param system_site_packages: If True, allow system packages in venv.
+"""
+logging.debug("%s: make_venv(venv_path=%s, system_site_packages=%s)",
+ __file__, str(venv_path), system_site_packages)
+venv.create(
+str(venv_path),
+clear=True,
+symlinks=os.name != "nt", # Same as venv CLI's default.
+with_pip=True,
+system_site_packages=system_site_packages,
+)
+
+
+@contextmanager
+def enter_venv(venv_dir: Union[str, Path]) -> Iterator[None]:
+"""Scoped activation of an existing venv."""
+venv_keys = ('PATH', 'PYTHONHOME', 'VIRTUAL_ENV')
+old = {k: v for k, v in os.environ.items() if k in venv_keys}
+vdir = Path(venv_dir).resolve()
+
+def _delete_keys() -> None:
+for k
[RFC PATCH v2 3/7] tests: Remove spurious pip warnings on Ubuntu20.04
The version of pip ("20.0.2") that ships with Ubuntu 20.04 has a bug
where it will try to attempt building a wheel even if the "wheel" python
package that enables it to do so is not installed. Even though pip
continues gracefully from source, The result is a lot of irrelevant
failure output.
Upstream pip 20.0.2 does not have this problem, and pip 20.1 introduces
a new info message that informs a user that wheel building is being skipped.
On this version, the output can be silenced by passing --no-binary to
coax pip into skipping that step to begin with. Note, this error does
not seem to show up for the "qemu" package because we install that
package in editable mode. (I think, but did not test, that installing an
empty package in editable mode caused more problems than it fixed.)
Signed-off-by: John Snow
---
tests/mkvenv.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/mkvenv.py b/tests/mkvenv.py
index 0667106d6aa..78f8d0382e0 100644
--- a/tests/mkvenv.py
+++ b/tests/mkvenv.py
@@ -144,7 +144,8 @@ def make_qemu_venv(
with enter_venv(venv_path):
if do_initialize:
install("-e", str(pysrc_path), offline=offline)
-install(str(test_src_path), offline=offline)
+install("--no-binary", "qemu.dummy-tests",
+str(test_src_path), offline=offline)
venv_path.touch()
for option in options:
--
2.34.3
[RFC PATCH v2 6/7] iotests: use tests/venv for running tests
Essentially, the changes to testenv.py here mimic the changes that occur when
you "source venv/bin/activate.fish" or similar.
(1) update iotest's internal notion of which python binary to use,
(2) export the VIRTUAL_ENV variable,
(3) front-load the venv/bin directory to PATH.
If the venv directory isn't found, raise a friendly exception that tries
to give the human operator a friendly clue as to what's gone wrong. The
subsequent commit attempts to address this shortcoming by teaching
iotests how to invoke the venv bootstrapper in this circumstance
instead.
Signed-off-by: John Snow
---
tests/qemu-iotests/testenv.py | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index a864c74b123..29404ac94be 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -65,8 +65,9 @@ class TestEnv(ContextManager['TestEnv']):
# lot of them. Silence pylint:
# pylint: disable=too-many-instance-attributes
-env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
- 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
+env_variables = ['PYTHONPATH', 'VIRTUAL_ENV', 'PYTHON', 'PATH',
+ 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
+ 'QEMU_PROG', 'QEMU_IMG_PROG',
'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
'QEMU_IO_OPTIONS', 'QEMU_IO_OPTIONS_NO_FMT',
@@ -102,18 +103,29 @@ def get_env(self) -> Dict[str, str]:
def init_directories(self) -> None:
"""Init directory variables:
+ VIRTUAL_ENV
+ PATH
PYTHONPATH
TEST_DIR
SOCK_DIR
SAMPLE_IMG_DIR
"""
+venv_path = Path(self.build_root, 'tests/venv/')
+if not venv_path.exists():
+raise FileNotFoundError(
+f"Virtual environment \"{venv_path!s}\" isn't found."
+" (Maybe you need to run 'make check-venv'"
+" from the build dir?)"
+)
+self.virtual_env: str = str(venv_path)
-# Path where qemu goodies live in this source tree.
-qemu_srctree_path = Path(__file__, '../../../python').resolve()
+self.path = os.pathsep.join((
+str(venv_path / 'bin'),
+os.environ['PATH'],
+))
self.pythonpath = os.pathsep.join(filter(None, (
self.source_iotests,
-str(qemu_srctree_path),
os.getenv('PYTHONPATH'),
)))
@@ -138,7 +150,7 @@ def init_binaries(self) -> None:
PYTHON (for bash tests)
QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, QSD_PROG
"""
-self.python = sys.executable
+self.python: str = os.path.join(self.virtual_env, 'bin', 'python3')
def root(*names: str) -> str:
return os.path.join(self.build_root, *names)
@@ -300,6 +312,7 @@ def print_env(self, prefix: str = '') -> None:
{prefix}GDB_OPTIONS -- {GDB_OPTIONS}
{prefix}VALGRIND_QEMU -- {VALGRIND_QEMU}
{prefix}PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
+{prefix}VIRTUAL_ENV -- {VIRTUAL_ENV}
{prefix}"""
args = collections.defaultdict(str, self.get_env())
--
2.34.3
[RFC PATCH v2 5/7] tests: add 'check-venv' as a dependency of 'make check'
This patch adds the 'check-venv' target as a requisite of all meson driven check-* targets. As of this commit, it will only install the "qemu" namespace package from the source tree, and nothing else. In the future, the "qemu" namespace package in qemu.git will begin to require an external qemu.qmp package, and this would be installed into this environment as well. The avocado test dependencies will *not* be pulled into this venv by default, but they may be added in at a later point in time by running 'make check-avocado' or, without running the tests, 'make check-venv-avocado'. Signed-off-by: John Snow --- tests/Makefile.include | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Makefile.include b/tests/Makefile.include index d8af6a38112..d484a335be5 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -155,6 +155,9 @@ check-acceptance-deprecated-warning: check-acceptance: check-acceptance-deprecated-warning | check-avocado +# The do-meson-check and do-meson-bench targets are defined in Makefile.mtest +do-meson-check do-meson-bench: check-venv + # Consolidated targets .PHONY: check check-clean get-vm-images -- 2.34.3
[RFC PATCH v2 4/7] tests/vm: add venv pre-requisites to VM building recipes
Ubuntu needs "python3-venv" in order to create virtual environments, and
NetBSD needs "py37-pip" in order to do the same.
Signed-off-by: John Snow
---
tests/vm/netbsd | 1 +
tests/vm/ubuntu.i386 | 9 ++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/tests/vm/netbsd b/tests/vm/netbsd
index 45aa9a7fda7..53fe508e487 100755
--- a/tests/vm/netbsd
+++ b/tests/vm/netbsd
@@ -31,6 +31,7 @@ class NetBSDVM(basevm.BaseVM):
"pkgconf",
"xz",
"python37",
+"py37-pip",
"ninja-build",
# gnu tools
diff --git a/tests/vm/ubuntu.i386 b/tests/vm/ubuntu.i386
index 47681b6f87d..40fd5ec86da 100755
--- a/tests/vm/ubuntu.i386
+++ b/tests/vm/ubuntu.i386
@@ -16,9 +16,12 @@ import basevm
import ubuntuvm
DEFAULT_CONFIG = {
-'install_cmds' : "apt-get update,"\
- "apt-get build-dep -y qemu,"\
- "apt-get install -y libfdt-dev language-pack-en
ninja-build",
+'install_cmds' : (
+"apt-get update,"
+"apt-get build-dep -y qemu,"
+"apt-get install -y libfdt-dev language-pack-en ninja-build,"
+"apt-get install -y python3-venv"
+),
}
class UbuntuX86VM(ubuntuvm.UbuntuVM):
--
2.34.3
[RFC PATCH v2 7/7] iotests: self-bootstrap testing venv
When iotests are invoked manually from
e.g. $build/tests/qemu-iotests/check, it is not necessarily guaranteed
that we'll have run 'check-venv' yet.
With this patch, teach testenv.py how to create its own environment.
Note: this self-bootstrapping is fairly rudimentary and will miss
certain triggers to refresh the venv. It will miss when new dependencies
are added to either python/setup.cfg or tests/setup.cfg. It can be
coaxed into updating by running 'make check', 'make check-block', 'make
check-venv', etc.
Signed-off-by: John Snow
---
tests/qemu-iotests/testenv.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 29404ac94be..e985eaf3e97 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -112,10 +112,10 @@ def init_directories(self) -> None:
"""
venv_path = Path(self.build_root, 'tests/venv/')
if not venv_path.exists():
-raise FileNotFoundError(
-f"Virtual environment \"{venv_path!s}\" isn't found."
-" (Maybe you need to run 'make check-venv'"
-" from the build dir?)"
+mkvenv = Path(self.source_iotests, '../mkvenv.py')
+subprocess.run(
+(sys.executable, str(mkvenv), str(venv_path)),
+check=True,
)
self.virtual_env: str = str(venv_path)
--
2.34.3
[RFC PATCH v2 0/7] tests: run python tests under a venv
Hi, here's another RFC for bringing external Python dependencies to the QEMU test suite. This patchset is not without some problems that need to be solved, but I've been sitting on these long enough and they need to see the light of day. Problems I am aware of, and there's a few: - Ubuntu 18.04 ships with a version of pip that is too old to support setup.cfg-based installations. We are allowed to drop support for 18.04 by now, but we need a suitable 32bit debian VM configuration to replace it. - Multiple VM tests are still failing for me; but they fail with or without my patches as far as I can tell. I'm having problems with Haiku and CentOS, primarily -- which I think fail even without my patches. I'll have more info after the weekend, these tests are SLOW. - This version of the patch series does not itself enforce any offline-only behavior for venv creation, but downstreams can modify any call to 'mkvenv' to pass '--offline'. A more flexible approach might be to allow an environment variable to be passed that toggles the switch on. - iotests will now actually never run mypy or pylint tests by default anymore, because the bootstrapper won't select those packages by default, and the virtual environment won't utilize the system packages -- so iotest 297 will just "skip" all of the time now. The reason we don't want to install these packages by default is because we don't want to add dependencies on mypy and pylint for downstream builds. With these patches, 297 would still work if you manually opened up the testing venv and installed suitable mypy/pylint packages there. I could also add a new optional dependency group, and one could theoretically invoke a once-per-build-dir command of 'make check-venv-pylint' to help make the process only semi-manual, but it's still annoying. Ideally, the python checks in qemu.git/python/ can handle the same tests as 297 does -- but we need to give a shorthand invocation like "make check-python" that is excluded from the default "make check" to allow block developers to quickly opt-in to the same tests. I've covered some of the problems here on-list before: https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg03661.html ...But I haven't quite solved them yet. That's all for now. Paolo, can we chat about build system integration next? I want to know how you envision the integration at this point -- adding different test-invocation styles (online, offline, etc) may help solve the iotest 297 problem and the iotest self-bootstrap problem. --js John Snow (7): tests: create optional tests/venv dependency groups tests: pythonize test venv creation tests: Remove spurious pip warnings on Ubuntu20.04 tests/vm: add venv pre-requisites to VM building recipes tests: add 'check-venv' as a dependency of 'make check' iotests: use tests/venv for running tests iotests: self-bootstrap testing venv tests/Makefile.include| 32 +++--- tests/mkvenv.py | 187 ++ tests/qemu-iotests/testenv.py | 25 +++-- tests/requirements.txt| 6 -- tests/setup.cfg | 20 tests/setup.py| 16 +++ tests/vm/netbsd | 1 + tests/vm/ubuntu.i386 | 9 +- 8 files changed, 268 insertions(+), 28 deletions(-) create mode 100644 tests/mkvenv.py delete mode 100644 tests/requirements.txt create mode 100644 tests/setup.cfg create mode 100644 tests/setup.py -- 2.34.3
Re: [PATCH] iotests: fix source directory location
On Thu, May 26, 2022 at 10:21 AM John Snow wrote: > > > > On Thu, May 26, 2022, 3:54 AM Daniel P. Berrangé wrote: >> >> On Wed, May 25, 2022 at 08:25:12PM -0400, John Snow wrote: >> > If you invoke the check script from outside of the tests/qemu-iotests >> > directory, the directories initialized as source_iotests and >> > build_iotests will be incorrect. >> > >> > We can use the location of the source file itself to be more accurate. >> > >> > Signed-off-by: John Snow >> > Reviewed-by: Paolo Bonzini >> > --- >> > tests/qemu-iotests/testenv.py | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py >> > index a864c74b123..9b0f01e84db 100644 >> > --- a/tests/qemu-iotests/testenv.py >> > +++ b/tests/qemu-iotests/testenv.py >> > @@ -217,10 +217,10 @@ def __init__(self, imgfmt: str, imgproto: str, >> > aiomode: str, >> > self.build_iotests = >> > os.path.dirname(os.path.abspath(sys.argv[0])) >> > else: >> > # called from the source tree >> > -self.source_iotests = os.getcwd() >> > +self.source_iotests = str(Path(__file__, '..').resolve()) >> >> Path(__file__).parent >> >> > self.build_iotests = self.source_iotests >> > >> > -self.build_root = os.path.join(self.build_iotests, '..', '..') >> > +self.build_root = str(Path(self.build_iotests, '../..').resolve()) >> >> Path(self.build_iotests).parent.parent >> >> to be portable > > > With windows? I think Path() is meant to be a fully portable class as-is, but > I'll double-check my assumption. I use ".." elsewhere in code already checked > in, so if it's a problem I ought to fix it everywhere. Found a Windows box, it works there too. Good enough? --js
Re: [PATCH 1/2] python/machine.py: upgrade vm.command() method
On Thu, May 26, 2022, 10:31 AM Vladimir Sementsov-Ogievskiy <
vsement...@yandex-team.ru> wrote:
> On 4/8/22 20:02, Vladimir Sementsov-Ogievskiy wrote:
> > The method is not popular, we prefer use vm.qmp() and then check
>
> Suddenly I found, that I missed a lot of existing users: in scripts, in
> avocado tests.
>
> Do you prefer to rename the method to "cmd()", and change all the
> occurrences, or keep longer "command()" name and update the second patch?
>
I don't have a strong preference, I think.
In (async) qmp I use .execute() as the form that raises exception, and
._raw() as the form that doesn't. I use execute_msg() as an
exception-raising form that takes a Mapping[str, obj].
Notably, I tried to hide any interface that didn't raise exception, and the
interfaces that remain always return the inner return field and not the
entire wire object.
command() IIRC has historically been the exception-raising version and
cmd() has been the C-like version. (cmd_obj works like my execute_msg,
except it doesn't raise, and returns the entire reply.)
command() is longer, but there's precedent and continuity for it working
this way. But shorter names are nicer for line length, so...
...Go with what you feel is subjectively nicest? (That's not helpful,
sorry.)
oh, also: IIRC, command() also does not return the entire response object.
This is how execute() works, but it might be a lot of churn to convert
users of cmd() over to this form. It's something I want to do eventually
anyway, but it's a lot for me to dump on your plate, so don't worry about
that aspect.
--js
>
> $ git grep '\.command('
> docs/devel/testing.rst: res =
> self.vm.command('human-monitor-command',
> docs/devel/testing.rst: first_res = first_machine.command(
> docs/devel/testing.rst: second_res = second_machine.command(
> docs/devel/testing.rst: third_res =
> self.get_vm(name='third_machine').command(
> python/qemu/machine/machine.py:ret = self._qmp.command(cmd,
> **qmp_args)
> python/qemu/utils/qemu_ga_client.py:return
> self.command('guest-' + name.replace('_', '-'), **kwds)
> python/qemu/utils/qom.py:rsp = self.qmp.command(
> python/qemu/utils/qom.py:rsp = self.qmp.command(
> python/qemu/utils/qom.py:rsp = self.qmp.command('qom-get',
> path=path,
> python/qemu/utils/qom_common.py:rsp = self.qmp.command('qom-list',
> path=path)
> python/qemu/utils/qom_fuse.py:data =
> str(self.qmp.command('qom-get', path=path, property=prop))
> python/qemu/utils/qom_fuse.py:return prefix +
> str(self.qmp.command('qom-get', path=path,
> scripts/device-crash-test:types = vm.command('qom-list-types',
> **kwargs)
> scripts/device-crash-test:devhelp =
> vm.command('human-monitor-command', **args)
> scripts/device-crash-test:self.machines = list(m['name'] for m
> in vm.command('query-machines'))
> scripts/device-crash-test:self.kvm_available =
> vm.command('query-kvm')['enabled']
> scripts/render_block_graph.py:bds_nodes =
> qmp.command('query-named-block-nodes')
> scripts/render_block_graph.py:job_nodes =
> qmp.command('query-block-jobs')
> scripts/render_block_graph.py:block_graph =
> qmp.command('x-debug-query-block-graph')
> tests/avocado/avocado_qemu/__init__.py:res =
> self.vm.command('human-monitor-command',
> tests/avocado/cpu_queries.py:cpus =
> self.vm.command('query-cpu-definitions')
> tests/avocado/cpu_queries.py:e =
> self.vm.command('query-cpu-model-expansion', model=model, type='full')
> tests/avocado/hotplug_cpu.py:self.vm.command('device_add',
> tests/avocado/info_usernet.py:res =
> self.vm.command('human-monitor-command',
> tests/avocado/machine_arm_integratorcp.py:
> self.vm.command('human-monitor-command', command_line='stop')
> tests/avocado/machine_arm_integratorcp.py:
> self.vm.command('human-monitor-command',
> tests/avocado/machine_m68k_nextcube.py:
> self.vm.command('human-monitor-command',
> tests/avocado/machine_mips_malta.py:
> self.vm.command('human-monitor-command', command_line='stop')
> tests/avocado/machine_mips_malta.py:
> self.vm.command('human-monitor-command',
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('device_del', id='rn1')
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('device_del', id='rn2')
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('device_add', driver='virtio-net-ccw',
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('device_del', id='net_4711')
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('human-monitor-command', command_line='balloon 96')
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('human-monitor-command', command_line='balloon 128')
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('screendump', filename=ppmfile.name)
> tests/avocado/machine_s390_ccw_virtio.py:
> self.vm.command('object-add', qom_type='
Re: [PATCH] iotests: fix source directory location
On Thu, May 26, 2022, 3:54 AM Daniel P. Berrangé wrote: > On Wed, May 25, 2022 at 08:25:12PM -0400, John Snow wrote: > > If you invoke the check script from outside of the tests/qemu-iotests > > directory, the directories initialized as source_iotests and > > build_iotests will be incorrect. > > > > We can use the location of the source file itself to be more accurate. > > > > Signed-off-by: John Snow > > Reviewed-by: Paolo Bonzini > > --- > > tests/qemu-iotests/testenv.py | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tests/qemu-iotests/testenv.py > b/tests/qemu-iotests/testenv.py > > index a864c74b123..9b0f01e84db 100644 > > --- a/tests/qemu-iotests/testenv.py > > +++ b/tests/qemu-iotests/testenv.py > > @@ -217,10 +217,10 @@ def __init__(self, imgfmt: str, imgproto: str, > aiomode: str, > > self.build_iotests = > os.path.dirname(os.path.abspath(sys.argv[0])) > > else: > > # called from the source tree > > -self.source_iotests = os.getcwd() > > +self.source_iotests = str(Path(__file__, '..').resolve()) > > Path(__file__).parent > > > self.build_iotests = self.source_iotests > > > > -self.build_root = os.path.join(self.build_iotests, '..', '..') > > +self.build_root = str(Path(self.build_iotests, > '../..').resolve()) > > Path(self.build_iotests).parent.parent > > to be portable > With windows? I think Path() is meant to be a fully portable class as-is, but I'll double-check my assumption. I use ".." elsewhere in code already checked in, so if it's a problem I ought to fix it everywhere.
[PATCH] iotests: fix source directory location
If you invoke the check script from outside of the tests/qemu-iotests directory, the directories initialized as source_iotests and build_iotests will be incorrect. We can use the location of the source file itself to be more accurate. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini --- tests/qemu-iotests/testenv.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py index a864c74b123..9b0f01e84db 100644 --- a/tests/qemu-iotests/testenv.py +++ b/tests/qemu-iotests/testenv.py @@ -217,10 +217,10 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str, self.build_iotests = os.path.dirname(os.path.abspath(sys.argv[0])) else: # called from the source tree -self.source_iotests = os.getcwd() +self.source_iotests = str(Path(__file__, '..').resolve()) self.build_iotests = self.source_iotests -self.build_root = os.path.join(self.build_iotests, '..', '..') +self.build_root = str(Path(self.build_iotests, '../..').resolve()) self.init_directories() self.init_binaries() -- 2.34.1
Re: The fate of iotest 297
On Thu, May 19, 2022, 4:25 AM Daniel P. Berrangé wrote: > On Thu, May 19, 2022 at 09:54:56AM +0200, Kevin Wolf wrote: > > Am 18.05.2022 um 20:21 hat John Snow geschrieben: > > > To wire it up to "make check" by *default*, I believe I need to expand > the > > > configure script to poll for certain requisites and then create some > > > wrapper script of some kind that only engages the python tests if the > > > requisites were met ... and I lose some control over the mypy/pylint > > > versioning windows. I have to tolerate a wider versioning, or it'll > never > > > get run in practice. > > > > > > I have some reluctance to doing this, because pylint and mypy change so > > > frequently that I don't want "make check" to fail spuriously in the > future. > > > > > > (In practice, these failures occur 100% of the time when I am on > vacation.) > > > > So we seem to agree that it's something that we do expect to fail from > > time to time. Maybe this is how I could express my point better: If it's > > a hard failure, it should fail as early as possible - i.e. ideally > > before the developer sends a patch, but certainly before failing a pull > > request. > > At least with pylint we can make an explicit list of which lint > checks we want to run, so we should not get new failures when a > new pylint is released. If there are rare cases where we none > the less see a new failure from a new release, then so be it, > whoever hits it first can send a patch. IOW, I think we should > just enable pylint all the time with a fixed list of tests we > care about. Over time we can enable more of its checks when > desired. > Yeh, this might help a bit. If we use system packages by default, we'll also generally avoid using bleeding edge packages and I'll (generally) catch those myself via check-tox before people run into them organically. > I don't know enough about mypy to know if it can provide similar > level of control. Possibly the answer for "should we run it by default" > will be different for pylint vs mypy. > Yeah, we can probably do different things. mypy is actually much more stable than pylint IMO, it's probably actually okay to just let that one behave as-is. (I know I have a fix for 0.950 in my recent rfc series, but anecdotally I feel mypy changes behavior a lot less often than pylint. isort and flake8 have basically never ever broken on update for me, either.) Still, none of this is all that different from the case where > new GCC or CLang are released and developers find new warnings > have arrived. People just send patches when they hit this. > Given python is a core part of QEMU's dev tooling, I think it > is reasonable to expect developers to cope with this for python > too, as long as the frequency of problems is not unreasonably > high. > To some extent, though it's still a bummer to get warnings and errors that have nothing to do with your changes. I have made sure I test a wide matrix to the best of my ability, so it should be fine. I guess I'm just super conservative about it ... (Well, and even when I had the check-tox test set to allow failure, the yellow exclamation mark still annoyed people. I'm just keen to avoid more nastygrams.) > > > That said ... maybe I can add a controlled venv version of > "check-python" > > > and just have a --disable-check-python or something that spec files > can opt > > > into. Maybe that will work well enough? > > > > > > i.e. maybe configure can check for the presence of pip, the python venv > > > module (debian doesnt ship it standard...), and PyPI connectivity and > if > > > so, enables the test. Otherwise, we skip it. > > > > I think this should work. If detecting the right environment is hard, I > > don't think there is even a requirement to do so. You can make > > --enable-check-python the default and if people don't want it, they can > > explicitly disable it. (I understand that until you run 'make check', it > > doesn't make a difference anyway, so pure users would never have to > > change the option, right?) > > I think it should just be the default too. Contributors have to accept > that python is a core part of our project and we expect such code to > pass various python quality control tests, on the wide variety of OS > platforms we run on. > I meant that I'd have the default be "auto", but if you're arguing for the default to be "on", I suppose I could. I have a weak preference for keeping the min requisites for a no-option configure set small.
Re: The fate of iotest 297
On Wed, May 18, 2022, 12:37 PM Kevin Wolf wrote: > Am 18.05.2022 um 01:28 hat John Snow geschrieben: > > Hi Kevin, > > > > I remember that you wanted some minimum Niceness threshold in order to > > agree to me removing iotest 297. > > > > I've already moved it onto GitLab CI in the form of the > > check-python-pipenv job, but I recall you wanted to be able to run it > > locally as well before agreeing to axe 297. I remember that you didn't > > think that running "make check-pipenv" from the python directory was > > sufficiently Nice enough. > > > > Do you need it to be part of "make check", or are you OK with > > something like "make check-python" from the build directory? > > > > I have a bit more work to do if you want it to be part of 'make check' > > (if you happen to have the right packages installed), but it's pretty > > easy right now to give you a 'make check-python' (where I just > > forcibly install those packages to the testing venv.) > > Hm, what is the reason for 'make check-python' not being part of 'make > check'? > Oh, it just needs more logic so that it performs correctly in RPM building environments. As a manual test, I'm free to just grab stuff from PyPI and build a venv to some precise specification and automate it. This is how "check-pipenv" and "check-tox" work. The RPM environment can't dial out to PyPI, so it shouldn't try any venv-based tests by default. To wire it up to "make check" by *default*, I believe I need to expand the configure script to poll for certain requisites and then create some wrapper script of some kind that only engages the python tests if the requisites were met ... and I lose some control over the mypy/pylint versioning windows. I have to tolerate a wider versioning, or it'll never get run in practice. I have some reluctance to doing this, because pylint and mypy change so frequently that I don't want "make check" to fail spuriously in the future. (In practice, these failures occur 100% of the time when I am on vacation.) The gitlab ci job check-python-tox pulls whatever the latest and greatest are, and these jobs fail so constantly we had to mark the job as optional. The check-pipenv job by contrast is extremely stable (its still must-pass) because it can concoct its own lil' universe. So, I can add something to make check by default but it needs some scaffolding to skip the test based on environment, and I have some reliability concerns. Ultimately, I don't believe tolerating a wide matrix for mypy/pylint really adds any value to 297; it only really matters if a specific environment comes up green, and that a developer like you or I can replicate that test locally and quickly. That said ... maybe I can add a controlled venv version of "check-python" and just have a --disable-check-python or something that spec files can opt into. Maybe that will work well enough? i.e. maybe configure can check for the presence of pip, the python venv module (debian doesnt ship it standard...), and PyPI connectivity and if so, enables the test. Otherwise, we skip it. Something like that. > I'm currently running two things locally, 'make check' (which is the > generic one that everyone should run) and iotests (for which it is > reasonable enough that I need to run it separately because it's the > special thing for my own subsystem). > Pretty much exactly what I do. (Except I run the python tests these days, too.) > Now adding a third one 'make check-python' certainly isn't the end of > the world, but it's not really something that is tied to my subsystem > any more. Having to run test cases separately for other subsystems > doesn't really scale for me, so I would prefer not to start doing that. > I can usually get away with not running the more special tests of other > subsystems before the pull request because I'm unlikely to break things > in other subsystems, but Python style warnings are easy to get. > Reasonable. I already forget to run things like avocado and vm tests, and I am sympathetic to not wanting to expand the list of manually run tests. (What avocado and vm tests have in common is that they need to fetch stuff from the internet, which I am learning makes them unsuitable for make check, which must work without internet. """Coincidentally""", tests that require internet seem to break an awful lot more often because they are getting run a lot less and in fewer places.) > If we're going to have 'make check-python' separate, but CI checks it, > we'll get pull requests that don't pass it and would then only
Re: [RFC PATCH 9/9] iotests: use tests/venv for running tests
On Mon, May 16, 2022 at 3:41 AM Paolo Bonzini wrote: > > On 5/14/22 17:55, John Snow wrote: > > On Fri, May 13, 2022, 11:33 AM Paolo Bonzini > <mailto:pbonz...@redhat.com>> wrote: > > IIRC we have some cases (FreeBSD?) where only the python3.x executable > > is available. This is why we 1) default to Meson's Python 3 if neither > > --meson nor --python are passed, and 2) use the shebang you mention but > > with *non-executable* files, which Meson treats magically as "invoke > > with the Python interpreter that was used to launch me". > > > > pkg install python3 on fbsd 13.0-R gives you /usr/bin/python3 fwiw. do > > you know in what circumstances you get only a point release binary? > > Aha, tests/vm/freebsd installs python37, not python3. But I guess it's > still a plausible configuration for this packaging setup. > Just confirming here that if you do 'pkg install python37' and you have no 'python3' link, the venv package will still make 'python' and 'python3' links. I think it's likely best to use the 'python3' one.
The fate of iotest 297
Hi Kevin, I remember that you wanted some minimum Niceness threshold in order to agree to me removing iotest 297. I've already moved it onto GitLab CI in the form of the check-python-pipenv job, but I recall you wanted to be able to run it locally as well before agreeing to axe 297. I remember that you didn't think that running "make check-pipenv" from the python directory was sufficiently Nice enough. Do you need it to be part of "make check", or are you OK with something like "make check-python" from the build directory? I have a bit more work to do if you want it to be part of 'make check' (if you happen to have the right packages installed), but it's pretty easy right now to give you a 'make check-python' (where I just forcibly install those packages to the testing venv.) --js
Re: [RFC PATCH 9/9] iotests: use tests/venv for running tests
On Fri, May 13, 2022, 11:33 AM Paolo Bonzini wrote: > On 5/13/22 16:38, John Snow wrote: > > It *should*, because "#!/usr/bin/env python3" is the preferred shebang > > for Python scripts. > > > > https://peps.python.org/pep-0394/ <https://peps.python.org/pep-0394/> > > > > 'python3' "should" be available. 'python' may not be. > > > > Probably the "python" name in Makefile for TESTS_PYTHON should actually > > be "python3" as well. In practice, all permutations (python, python3, > > python3.9, etc.) are symlinks* to the binary used to create the venv. > > Which links are present may be site configurable, but pep394 should > > guarantee that python3 is always available. > > IIRC we have some cases (FreeBSD?) where only the python3.x executable > is available. This is why we 1) default to Meson's Python 3 if neither > --meson nor --python are passed, and 2) use the shebang you mention but > with *non-executable* files, which Meson treats magically as "invoke > with the Python interpreter that was used to launch me". > > Paolo > pkg install python3 on fbsd 13.0-R gives you /usr/bin/python3 fwiw. do you know in what circumstances you get only a point release binary? Creating a venv on fbsd with "python3 -m venv testvenv" created a python3 binary link, but not a python3.8 link, also. Still leaning towards the idea that "python3" is safest, but maybe it depends on how you install from ports etc. I'd still say that it's reasonable to expect that a system with python pays heed to PEP0394, I think you've got a broken python install if you don't. (But, what's the use case that forced your hand otherwise?) --js >
Re: [RFC PATCH 0/9] tests: run python tests under the build/tests/venv environment
On Fri, May 13, 2022, 12:50 PM Paolo Bonzini wrote: > On 5/13/22 18:07, Daniel P. Berrangé wrote: > >> e.g. what if I want to require mypy >= 0.900 for testing, but you have a > >> system package that requires mypy < 0.700? > > I would expect us to not require packages that are not present in > > the distros implied by > > > >https://www.qemu.org/docs/master/about/build-platforms.html > > > > if that was absolutely a must have, then gracefully skip tests > > if the system version wasn't new enough. The user could always > > pass --python-env=pip if they want to force new enough > > > > Consider that e.g. RHEL RPMs do not do mypy or pylint in %check, because > the version of the linters in RHEL is usually older than what the > upstream packages expect. > > I don't think it's a good idea for QEMU to support what Red Hat > packagers decided was a bad idea to support. > > Paolo > Yeah, I have to insist that due to the way these packages are developed upstream that it is simply not reasonable to expect that the local package version will work. pylint changes behavior virtually every single release. This series itself even has a patch that is playing whackamole to support a mypy that's brand new while supporting older mypy versions. It's a huge overhead for little gain. Far preferable to just say "Oh, your linter version is too old, we can't run this test locally." the qemu (and qemu.qmp) packages do not express a runtime/install dependency on mypy/pylint/isort/flake8/avocado/tox. These packages only get pulled in for the [devel] option group, and for good reason. What is really the outlier here is iotest 297, which brings a kind of meta-test into the same category as functional/regression tests. Supporting this on default system packages is not on my personal todo list. Moving this test off to a "make check-python" and deleting iotest 297 might be an option. This is a test that simply might need to be skipped by an SRPM build. (it already isn't run, so there's no additional harm by continuing to not run it.) If we are running a test suite and we allow pypi via the config, then I believe we ought to allow the pypi versions to supersede the system ones - *iff* the system ones are insufficient. I will continue to endeavor to test a very wide matrix of dependencies, I just have to be honest that I don't think I will reasonably achieve the full breadth you are asking for here. For no-internet configs, we may have to accept that some platforms simply don't have new enough dependencies to run some tests. I don't see this as violating our build platform promise. I don't believe the build platform promise ever reasonably extended to a "development platform promise". --js >
Re: [RFC PATCH 6/9] tests: add check-venv as a dependency of check and check-block
On Fri, May 13, 2022, 11:34 AM Paolo Bonzini wrote: > On 5/13/22 16:12, John Snow wrote: > > > > I think you need instead: > > > > # The do-meson-check and do-meson-bench targets are defined in > > Makefile.mtest > > do-meson-check do-meson-bench: check-venv > > > > and I would even add "all" to the targets that create the virtual > > environment. > > > > Paolo > > > > > > Great, thanks! I'll try that out today. > > Well, check out the other suggestion of creating the venv at configure > time, because that would remove all these complications/annoyances. > > Paolo > They also raise new annoyances and questions for me, so it might be worth updating this "branch" of the patchset to have a basis of comparison for what's the least annoying in the end. (Or maybe even to serve as a basis while transitioning to the "better" solution. It's quick to try, at least.) Config script ideas are gonna take me a bit longer to work through. (I'm not against developing a minimum viable patchset and having you tweak it to your desire afterwards, if you have the time/interest. We can chat on irc if you'd like. Otherwise, I'll just push forward.) --js >
Re: [RFC PATCH 9/9] iotests: use tests/venv for running tests
On Fri, May 13, 2022, 11:33 AM Paolo Bonzini wrote: > On 5/13/22 16:38, John Snow wrote: > > It *should*, because "#!/usr/bin/env python3" is the preferred shebang > > for Python scripts. > > > > https://peps.python.org/pep-0394/ <https://peps.python.org/pep-0394/> > > > > 'python3' "should" be available. 'python' may not be. > > > > Probably the "python" name in Makefile for TESTS_PYTHON should actually > > be "python3" as well. In practice, all permutations (python, python3, > > python3.9, etc.) are symlinks* to the binary used to create the venv. > > Which links are present may be site configurable, but pep394 should > > guarantee that python3 is always available. > > IIRC we have some cases (FreeBSD?) where only the python3.x executable > is available. This is why we 1) default to Meson's Python 3 if neither > --meson nor --python are passed, and 2) use the shebang you mention but > with *non-executable* files, which Meson treats magically as "invoke > with the Python interpreter that was used to launch me". > This tidbit is particularly 😥 > Paolo > FreeBSD, why do you insist on hurting me? (I'm surprised - python3 is *supposed* to be defined. Isn't this supremely annoying for FreeBSD users to have every last Python shebang script not work?) OK. I'll test, and possibly update avocado's existing makefile magic if necessary. It may be the case that the venv just works anyway. No way to know but to test, I guess.
Re: [RFC PATCH 0/9] tests: run python tests under the build/tests/venv environment
On Fri, May 13, 2022, 6:29 AM Daniel P. Berrangé wrote: > On Fri, May 13, 2022 at 09:35:23AM +0100, Daniel P. Berrangé wrote: > > On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote: > > > RFC: This is a very early, crude attempt at switching over to an > > > external Python package dependency for QMP. This series does not > > > actually make the switch in and of itself, but instead just switches to > > > the paradigm of using a venv in general to install the QEMU python > > > packages instead of using PYTHONPATH to load them from the source tree. > > > > > > (By installing the package, we can process dependencies.) > > > > > > I'm sending it to the list so I can show you some of what's ugly so far > > > and my notes on how I might make it less ugly. > > > > > > (1) This doesn't trigger venv creation *from* iotests, it merely prints > > > a friendly error message if "make check-venv" has not been run > > > first. Not the greatest. > > > > So if we run the sequence > > > > mkdir build > > cd build > > ../configure > > make > > ./tests/qemu-iotests/check 001 > > > > It won't work anymore, until we 'make check-venv' (or simply > > 'make check') ? > > > > I'm somewhat inclined to say that venv should be created > > unconditionally by default. ie a plain 'make' should always > > everything needed to be able to invoke the tests directly. > > > > > (2) This isn't acceptable for SRPM builds, because it uses PyPI to > fetch > > > packages just-in-time. My thought is to use an environment variable > like > > > QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup > > > process. We can use "--system-site-packages" as an argument to venv > > > creation and "--no-index" as an argument to pip installation to achieve > > > good behavior in SRPM building scenarios. It'd be up to the spec-writer > > > to opt into that behavior. > > > > I think I'd expect --system-site-packages to be the default behaviour. > > We expect QEMU to be compatible with the packages available in the > > distros that we're targetting. So if the dev has the python packages > > installed from their distro, we should be using them preferentially. > > > > This is similar to how we bundle slirp/capstone/etc, but will > > preferentially use the distro version if it is available. > > AFAICT from testing it, when '--system-site-packages' is set > for the venv, then 'pip install' appears to end up being a > no-op if the package is already present in the host, but > installs it if missing. > > IOW, if we default to --system-site-packages, but still > also run 'pip install', we should "do the right thing". > It'll use any distro packages that are available, and > augment with stuff from pip. In the no-op case, pip will > still try to consult the pipy servers to fetch version > info IIUC, so we need to be able to stop that. So I'd > suggest a --python-env arg taking three values > > * "auto" (the default) - add --system-site-packages, but >also run 'pip install'. Good for developers day-to-day > Sounds like a decent balance... ...My only concern is that the system packages might be very old and it's possible that the qemu packages will be "too new" or have conflicts with the system deps. I'll just have to test this. e.g. what if I want to require mypy >= 0.900 for testing, but you have a system package that requires mypy < 0.700? I don't *know* that this is a problem, but my python-sense is tingling. The python dep compatibility matrix I try to enforce and support for testing is already feeling overly wide. This might force me to support an even wider matrix, which I think is the precisely wrong direction for venvs where we want tighter control as a rule. > * "system" - add --system-site-packages but never run >'pip install'. Good for formal package builds. > We still have to install the in-tree qemu ns package, but we can use --no-index to do it. It'll fail if the deps aren't met. > * "pip" - don't add --system-site-packages, always run >'pip install'. Good for testing compatibility with >bleeding edge python versions, or if explicit full >independance from host python install is desired. > as arguments to configure, this spread of options makes sense to me than paolo's version, but I've still got some doubt on mixing system and venv packages. I am also as of yet not sold on building the venv *from* configure, see my response to Paolo on that topic. I'll keep plugging away for now, but the big picture is still a tad murky in my head. --js
Re: [RFC PATCH 0/9] tests: run python tests under the build/tests/venv environment
On Fri, May 13, 2022, 6:21 AM Paolo Bonzini wrote:
> On 5/13/22 02:06, John Snow wrote:
> > The only downside I am really frowning at is that I will have to
> > replicate some "update the venv if it's outdated" logic that is usually
> > handled by the Make system in the venv bootstrapper. Still, I think it's
> > probably the only way to hit all of the requirements here without trying
> > to concoct a fairly complex Makefile.
> >
> > any thoughts? If not, I'll just move on to trying to hack up that
> > version next instead.
>
> I would not even bother with keeping the venv up to date. Just initialize
>
I'm worried about this idea being very inconvenient for iterative
development of the python code.
it in configure, this is exactly what configure remains useful for in the
> Meson-based world:
>
> - add configure options --enable-python-qemu={enabled,system,internal,pip,
> auto}/--disable-python-qemu (auto means system>internal>pip>disabled;
> enabled means
> system>internal>pip>error) and matching CONFIG_PYTHON_QEMU=y to
> config-host.mak
>
I'm not sure this makes sense. python/qemu will continue to exist in-tree
and will only ever be "internal" in that sense. It won't be something you
can wholesale install from pip.
i.e. I plan to continue to break off pieces and upstream them, but I intend
to leave several modules as internal only.
So I'm not sure "internal" vs "pip" makes sense config-wise, it's intended
to be a mixture of both, really.
But, I suppose this is how you'd like to address different venv setup
behaviors to accommodate spec builds vs dev builds - with a configure flag
of some kind.
(I suppose you'd then like to see configure error out if it doesn't have
the necessary requisites given the venv-style chosen?)
- use CONFIG_PYTHON_QEMU to enable/disable iotests in
> tests/qemu-iotests/meson.build
>
So it's just skipped if you don't have the reqs to make the venv? (Not an
error?)
> - add a configure option --enable-avocado=
> {system,pip,auto,enabled}/--disable-avocado and matching
> CONFIG_AVOCADO=y to config-host.mak
>
> - use it to enable/disable acceptance tests in tests/Makefile.include
>
And this is how you propose eliminating the need for an always-present
avocado builddep.
> - build the venv in configure and use the options to pick the right pip
> install
> commands, like
>
> has_python_module() {
>$python -c "import $1" > /dev/null 2>&1
> }
>
> # do_pip VENV-PATH VAR PACKAGE [PATH] -- PIP-OPTIONS
> do_pip() {
> local num_args source
> num_args=5
> test $4 = '--' && num_args=4
> eval source=\$$2
> # Try to resolve the package using a system install
> case $source in
>enabled|auto|system)
> if has_python_module $3; then
>source=system
> elif test $source = system; then
>error_exit "Python package $3 not installed"
> fi
> esac
> # See if a bundled copy is present
> case $source in
>enabled|auto|internal)
> if test $num_args = 5 && test -f $4/setup.cfg; then
>source=internal
> elif test $source = internal; then
>error_exit "Sources for Python package $3 not found in the QEMU
> source tree"
> fi
> esac
> # Install the bundled copy or let pip download the package
> case $source in
>internal)
> # The Pip error message should be clear enough
> (cd $1 && . bin/activate && pip install "$@") || exit 1
>;;
>enabled|auto|pip)
> shift $num_args
> if (cd $1 && . bin/activate && pip install "$@"); then
>source=pip
> elif test $source = auto; then
>source=disabled
> else
># The Pip error message should be clear enough
>exit 1
> fi
>;;
> esac
> eval $2=\$source
> }
>
> rm -rf venv/
> $python -m venv venv/
> do_pip venv/ enable_python_qemu qemu.qmp python/qemu -- qemu.qmp
> do_pip venv/ enable_avocado avocado -- -r tests/requirements.txt
>
Won't this rebuild the venv like *all of the time*, basically whenever you
see the "configuration is stale" message?
That both seems like way too often, *and* it wouldn't cover cases when it
really ought to be refreshed.
Re: [RFC PATCH 0/9] tests: run python tests under the build/tests/venv environment
On Fri, May 13, 2022, 4:35 AM Daniel P. Berrangé wrote: > On Thu, May 12, 2022 at 08:06:00PM -0400, John Snow wrote: > > RFC: This is a very early, crude attempt at switching over to an > > external Python package dependency for QMP. This series does not > > actually make the switch in and of itself, but instead just switches to > > the paradigm of using a venv in general to install the QEMU python > > packages instead of using PYTHONPATH to load them from the source tree. > > > > (By installing the package, we can process dependencies.) > > > > I'm sending it to the list so I can show you some of what's ugly so far > > and my notes on how I might make it less ugly. > > > > (1) This doesn't trigger venv creation *from* iotests, it merely prints > > a friendly error message if "make check-venv" has not been run > > first. Not the greatest. > > So if we run the sequence > > mkdir build > cd build > ../configure > make > ./tests/qemu-iotests/check 001 > > It won't work anymore, until we 'make check-venv' (or simply > 'make check') ? > In this RFC as-is, that's correct. I want to fix that, because I dislike it too. Several ways to go about that. I'm somewhat inclined to say that venv should be created > unconditionally by default. ie a plain 'make' should always > everything needed to be able to invoke the tests directly. > I'm leaning to agree with you, but I see Kevin has some doubts. My #1 goal for Python refactoring is usually minimizing interruption to the block maintainers. I do like the idea of just having it always available and always taken care of, though. (This would be useful for making sure that any python scripts or utilities that need access to qmp/machine can be made to work, too. We can discuss this problem a little later - the scripts/qmp/ folder needs some work. It will come up in the full series to make the switch.) OTOH, A concern about unconditionally building the test venv is that it might introduce new dependencies for lots of downstreams that don't even run the tests yet. I think I am partial to having it install on-demand, because then the dependencies are opt-in. mjt told me that Debian does not run make check as part of its build yet, for example. I guess I can see it working either way. I think in the very immediate term I'm motivated to have it be on-demand, but long term I think "as part of make" is the eventual goal. > > (2) This isn't acceptable for SRPM builds, because it uses PyPI to fetch > > packages just-in-time. My thought is to use an environment variable like > > QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup > > process. We can use "--system-site-packages" as an argument to venv > > creation and "--no-index" as an argument to pip installation to achieve > > good behavior in SRPM building scenarios. It'd be up to the spec-writer > > to opt into that behavior. > > I think I'd expect --system-site-packages to be the default behaviour. > We expect QEMU to be compatible with the packages available in the > distros that we're targetting. So if the dev has the python packages > installed from their distro, we should be using them preferentially. > > This is similar to how we bundle slirp/capstone/etc, but will > preferentially use the distro version if it is available. > If you think that behavior should apply to tests as well, then OK. I shied away from having it as the default because it's somewhat unusual to "cede control" in a venv like this - the mere presence of certain packages in the system environment may change behavior of certain python libraries. It is a less well defined environment inherently. I'll do some testing and I can try having it always do this. I'm curious about cases where I might require "exactly mypy 0.780" and the user has mypy 0.770 installed, or maybe even the other way around. It may be surprising as to when the system packages get used and when they don't - instinctively I like things that are less dynamic, but I see the argument for wanting to prefer system packages when possible. At least for the sake of downstream. (I kind of feel like upstream should likewise prefer the upstream python packages too, but ... You've got a lot more packaging experience than me, so I'm willing to trust you on this point, but I'm personally a little uncertain.) > > (3) Using one venv for *all* tests means that avocado comes as a pre-req > > for iotests -- which adds avocado as a BuildRequires for the Fedora > > SRPM. That's probably not ideal. It may be better to model the test venv > > as someth
Re: [RFC PATCH 9/9] iotests: use tests/venv for running tests
On Fri, May 13, 2022, 4:38 AM Paolo Bonzini wrote:
> On 5/13/22 02:06, John Snow wrote:
> > Essentially, this:
> >
> > (A) adjusts the python binary to be the one found in the venv (which is
> > a symlink to the python binary chosen at configure time)
> >
> > (B) adds a new VIRTUAL_ENV export variable
> >
> > (C) changes PATH to front-load the venv binary directory.
> >
(amending my commit message/rfc notes while I'm here:)
I'll add that this way of entering a venv is more complex than the method
used for VM tests and Avocado tests because it allows the possibility of
shell tests (et al) invoking python utilities and having those be "in the
venv" as well.
i.e. it's more rigorous and it matches the behavior of the "activate" shell
script bundled with every venv.
> > If the venv directory isn't found, raise a friendly exception that tries
> > to give the human operator a friendly clue as to what's gone wrong. In
> > the very near future, I'd like to teach iotests how to fix this problem
> > entirely of its own volition, but that's a trick for a little later.
> >
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/testenv.py | 24 +---
> > 1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > index 0007da3f06c..fd3720ed7e7 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -65,8 +65,9 @@ class TestEnv(ContextManager['TestEnv']):
> > # lot of them. Silence pylint:
> > # pylint: disable=too-many-instance-attributes
> >
> > -env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR',
> 'SAMPLE_IMG_DIR',
> > - 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
> > +env_variables = ['PYTHONPATH', 'VIRTUAL_ENV', 'PYTHON',
> > + 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
> > + 'QEMU_PROG', 'QEMU_IMG_PROG',
> >'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
> >'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
> >'QEMU_IO_OPTIONS', 'QEMU_IO_OPTIONS_NO_FMT',
> > @@ -98,6 +99,10 @@ def get_env(self) -> Dict[str, str]:
> > if val is not None:
> > env[v] = val
> >
> > +env['PATH'] = os.pathsep.join((
> > +os.path.join(self.virtual_env, 'bin'),
> > +os.environ['PATH']
> > +))
> > return env
> >
> > def init_directories(self) -> None:
> > @@ -107,13 +112,17 @@ def init_directories(self) -> None:
> >SOCK_DIR
> >SAMPLE_IMG_DIR
> > """
> > -
> > -# Path where qemu goodies live in this source tree.
> > -qemu_srctree_path = Path(__file__, '../../../python').resolve()
> > +venv_path = Path(self.build_root, 'tests/venv/')
> > +if not venv_path.exists():
> > +raise FileNotFoundError(
> > +f"Virtual environment \"{venv_path!s}\" isn't found."
> > +" (Maybe you need to run 'make check-venv'"
> > +" from the build dir?)"
> > +)
> > +self.virtual_env: str = str(venv_path)
> >
> > self.pythonpath = os.pathsep.join(filter(None, (
> > self.source_iotests,
> > -str(qemu_srctree_path),
> > os.getenv('PYTHONPATH'),
> > )))
> >
> > @@ -138,7 +147,7 @@ def init_binaries(self) -> None:
> >PYTHON (for bash tests)
> >QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG,
> QSD_PROG
> > """
> > -self.python = sys.executable
> > +self.python: str = os.path.join(self.virtual_env, 'bin',
> 'python3')
>
> Is this guaranteed even if, say, only a /usr/bin/python3.9 exists?
> os.path.basename(sys.executable) might be more weirdness-proof than
> 'python3'.
>
> Paolo
>
It *should*, because "#!/usr/bin/env python3" is the preferred shebang for
Python scripts.
https://peps.python.org/pep-0394/
'python3' "should" be available. 'python' may not be.
Probably the "python" name in Makefile for TESTS_PYTHON should actually be
"python3" as well. In practice, all permutations (python, python3,
python3.9, etc.) are symlinks* to the binary used to create the venv. Which
links are present may be site configurable, but pep394 should guarantee
that python3 is always available.
(* not on Windows, where it'll be a copy.)
>
Re: [RFC PATCH 6/9] tests: add check-venv as a dependency of check and check-block
On Fri, May 13, 2022, 4:41 AM Paolo Bonzini wrote: > On 5/13/22 02:06, John Snow wrote: > > meson, create the python venv for block tests. > > +.PHONY: check-block > > +check-block: check-venv > > + @echo # Without some rule, this doesn't run at all. Why? > > + > > + > > # Consolidated targets > > > > .PHONY: check check-clean get-vm-images > > -check: > > +check: check-venv > > + @echo # ??? > > > > I think you need instead: > > # The do-meson-check and do-meson-bench targets are defined in > Makefile.mtest > do-meson-check do-meson-bench: check-venv > > and I would even add "all" to the targets that create the virtual > environment. > > Paolo > Great, thanks! I'll try that out today.
Re: [RFC PATCH 1/9] python: update for mypy 0.950
On Fri, May 13, 2022, 4:42 AM Paolo Bonzini wrote: > On 5/13/22 02:06, John Snow wrote: > > typeshed (included in mypy) recently updated to improve the typing for > > WriteTransport objects. I was working around this, but now there's a > > version where I shouldn't work around it. > > > > Unfortunately this creates some minor ugliness if I want to support both > > pre- and post-0.950 versions. For now, for my sanity, just disable the > > unused-ignores warning. > > > > Signed-off-by: John Snow > > Whatever floats your boat :) > > Reviewed-by: Paolo Bonzini > > Paolo > Maybe I'll move towards pinning specific versions of analysis tools once we move to always using a venv, and I won't have to try so hard to target a wide spread of versions for mypy, pylint, etc. I've tried pretty hard to "just have it work", but with the prevailing idioms in the Python world being what they are, I am playing whackamole virtually every release. But, yeah, for now... meh. This keeps the boat afloat.
Re: [RFC PATCH 4/9] tests: silence pip upgrade warnings during venv creation
On Fri, May 13, 2022, 4:27 AM Paolo Bonzini wrote: > On 5/13/22 02:06, John Snow wrote: > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > index dbbf1ba535b..dfb678d379f 100644 > > --- a/tests/Makefile.include > > +++ b/tests/Makefile.include > > @@ -109,11 +109,11 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) > $(SRC_PATH)/python/setup.cfg > > $(PYTHON) -m venv $@, \ > > VENV, $@) > > $(call quiet-command, \ > > -$(TESTS_PYTHON) -m pip -q install \ > > +$(TESTS_PYTHON) -m pip -q --disable-pip-version-check > install \ > > -e "$(SRC_PATH)/python/", PIP, "$(SRC_PATH)/python/") > > $(call quiet-command, \ > > -$(TESTS_PYTHON) -m pip -q install -r $(TESTS_VENV_REQ), \ > > -PIP, $(TESTS_VENV_REQ)) > > +$(TESTS_PYTHON) -m pip -q --disable-pip-version-check > install \ > > +-r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ)) > > $(call quiet-command, touch $@) > > Really nitpicking but I would have placed this change before adding the > second invocation of pip. :) > > Paolo > You're right. This RFC was a little disorganized, I wasn't sure I was going to keep any of this code just yet, so it missed a cleanup pass. (Forgive me, please!)
Re: [RFC PATCH 3/9] tests: install "qemu" namespace package into venv
On Fri, May 13, 2022, 4:26 AM Paolo Bonzini wrote: > On 5/13/22 02:06, John Snow wrote: > > diff --git a/tests/requirements.txt b/tests/requirements.txt > > index a21b59b4439..0ba561b6bdf 100644 > > --- a/tests/requirements.txt > > +++ b/tests/requirements.txt > > @@ -1,5 +1,6 @@ > > # Add Python module requirements, one per line, to be installed > > # in the tests/venv Python virtual environment. For more info, > > # refer to: https://pip.pypa.io/en/stable/user_guide/#id1 > > +# Note that qemu.git/python/ is always implicitly installed. > > avocado-framework==88.1 > > pycdlib==1.11.0 > > Any reason not to put ./python here? But anyway, > > Reviewed-by: Paolo Bonzini > > Paolo > The path didn't work under all circumstances - I got some bad path errors for some permutations of CWD/build-type. And I was not able to combine -e (for qemu) and -r (for this file) in a single command, so I kept the qemu install separate/special. Not ideal, I do admit. (I wanted -e for the in-tree install to not create a potential future landmine for someone changing python code and then getting confused as to why nothing changed when running e.g. iotests.)
[RFC PATCH 8/9] iotests: fix source directory location
If you invoke the check script from outside of the tests/qemu-iotests directory, the directories initialized as source_iotests and build_iotests will be incorrect. We can use the location of the source file itself to be more accurate. (I don't know if this is actually *used*, but what was there was wrong, I think.) Signed-off-by: John Snow --- tests/qemu-iotests/testenv.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py index a864c74b123..0007da3f06c 100644 --- a/tests/qemu-iotests/testenv.py +++ b/tests/qemu-iotests/testenv.py @@ -217,10 +217,10 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str, self.build_iotests = os.path.dirname(os.path.abspath(sys.argv[0])) else: # called from the source tree -self.source_iotests = os.getcwd() +self.source_iotests = str(Path(__file__, '../').resolve()) self.build_iotests = self.source_iotests -self.build_root = os.path.join(self.build_iotests, '..', '..') +self.build_root = str(Path(self.build_iotests, '../..').resolve()) self.init_directories() self.init_binaries() -- 2.34.1
[RFC PATCH 7/9] tests: add check-venv to build-tcg-disabled CI recipe
Signed-off-by: John Snow --- .gitlab-ci.d/buildtest.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml index 0aea7ab84c2..5c6201847f1 100644 --- a/.gitlab-ci.d/buildtest.yml +++ b/.gitlab-ci.d/buildtest.yml @@ -245,6 +245,7 @@ build-tcg-disabled: - make -j"$JOBS" - make check-unit - make check-qapi-schema +- make check-venv - cd tests/qemu-iotests/ - ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048 052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163 -- 2.34.1
[RFC PATCH 4/9] tests: silence pip upgrade warnings during venv creation
Turn off the nag warning coaxing us to upgrade pip. It's not really that interesting to see in CI logs, and as long as nothing is broken -- nothing is broken. Signed-off-by: John Snow --- tests/Makefile.include | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index dbbf1ba535b..dfb678d379f 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -109,11 +109,11 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) $(SRC_PATH)/python/setup.cfg $(PYTHON) -m venv $@, \ VENV, $@) $(call quiet-command, \ -$(TESTS_PYTHON) -m pip -q install \ +$(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \ -e "$(SRC_PATH)/python/", PIP, "$(SRC_PATH)/python/") $(call quiet-command, \ -$(TESTS_PYTHON) -m pip -q install -r $(TESTS_VENV_REQ), \ -PIP, $(TESTS_VENV_REQ)) +$(TESTS_PYTHON) -m pip -q --disable-pip-version-check install \ +-r $(TESTS_VENV_REQ), PIP, $(TESTS_VENV_REQ)) $(call quiet-command, touch $@) $(TESTS_RESULTS_DIR): -- 2.34.1
[RFC PATCH 3/9] tests: install "qemu" namespace package into venv
This patch adds the "qemu" namespace package to the $build/tests/venv directory. It does so in "editable" mode, which means that changes to the source python directory will actively be reflected by the venv. This patch also then removes any sys.path hacking from the avocado test scripts directly. By doing this, the environment of where to find these packages is managed entirely by the virtual environment and not by the scripts themselves. Signed-off-by: John Snow --- tests/Makefile.include | 5 - tests/avocado/avocado_qemu/__init__.py | 11 +-- tests/avocado/virtio_check_params.py | 1 - tests/avocado/virtio_version.py| 1 - tests/requirements.txt | 1 + 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index 146aaa96a00..dbbf1ba535b 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -104,10 +104,13 @@ else AVOCADO_CMDLINE_TAGS=$(addprefix -t , $(AVOCADO_TAGS)) endif -$(TESTS_VENV_DIR): $(TESTS_VENV_REQ) +$(TESTS_VENV_DIR): $(TESTS_VENV_REQ) $(SRC_PATH)/python/setup.cfg $(call quiet-command, \ $(PYTHON) -m venv $@, \ VENV, $@) + $(call quiet-command, \ +$(TESTS_PYTHON) -m pip -q install \ +-e "$(SRC_PATH)/python/", PIP, "$(SRC_PATH)/python/") $(call quiet-command, \ $(TESTS_PYTHON) -m pip -q install -r $(TESTS_VENV_REQ), \ PIP, $(TESTS_VENV_REQ)) diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py index 39f15c1d518..b656a70c55b 100644 --- a/tests/avocado/avocado_qemu/__init__.py +++ b/tests/avocado/avocado_qemu/__init__.py @@ -21,6 +21,11 @@ from avocado.utils import cloudinit, datadrainer, process, ssh, vmimage from avocado.utils.path import find_command +from qemu.machine import QEMUMachine +from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available, +tcg_available) + + #: The QEMU build root directory. It may also be the source directory #: if building from the source dir, but it's safer to use BUILD_DIR for #: that purpose. Be aware that if this code is moved outside of a source @@ -35,12 +40,6 @@ else: SOURCE_DIR = BUILD_DIR -sys.path.append(os.path.join(SOURCE_DIR, 'python')) - -from qemu.machine import QEMUMachine -from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available, -tcg_available) - def has_cmd(name, args=None): """ diff --git a/tests/avocado/virtio_check_params.py b/tests/avocado/virtio_check_params.py index e869690473a..4093da8a674 100644 --- a/tests/avocado/virtio_check_params.py +++ b/tests/avocado/virtio_check_params.py @@ -22,7 +22,6 @@ import re import logging -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) from qemu.machine import QEMUMachine from avocado_qemu import QemuSystemTest from avocado import skip diff --git a/tests/avocado/virtio_version.py b/tests/avocado/virtio_version.py index 208910bb844..c84e48813a1 100644 --- a/tests/avocado/virtio_version.py +++ b/tests/avocado/virtio_version.py @@ -11,7 +11,6 @@ import sys import os -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) from qemu.machine import QEMUMachine from avocado_qemu import QemuSystemTest diff --git a/tests/requirements.txt b/tests/requirements.txt index a21b59b4439..0ba561b6bdf 100644 --- a/tests/requirements.txt +++ b/tests/requirements.txt @@ -1,5 +1,6 @@ # Add Python module requirements, one per line, to be installed # in the tests/venv Python virtual environment. For more info, # refer to: https://pip.pypa.io/en/stable/user_guide/#id1 +# Note that qemu.git/python/ is always implicitly installed. avocado-framework==88.1 pycdlib==1.11.0 -- 2.34.1
[RFC PATCH 9/9] iotests: use tests/venv for running tests
Essentially, this:
(A) adjusts the python binary to be the one found in the venv (which is
a symlink to the python binary chosen at configure time)
(B) adds a new VIRTUAL_ENV export variable
(C) changes PATH to front-load the venv binary directory.
If the venv directory isn't found, raise a friendly exception that tries
to give the human operator a friendly clue as to what's gone wrong. In
the very near future, I'd like to teach iotests how to fix this problem
entirely of its own volition, but that's a trick for a little later.
Signed-off-by: John Snow
---
tests/qemu-iotests/testenv.py | 24 +---
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 0007da3f06c..fd3720ed7e7 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -65,8 +65,9 @@ class TestEnv(ContextManager['TestEnv']):
# lot of them. Silence pylint:
# pylint: disable=too-many-instance-attributes
-env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
- 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
+env_variables = ['PYTHONPATH', 'VIRTUAL_ENV', 'PYTHON',
+ 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
+ 'QEMU_PROG', 'QEMU_IMG_PROG',
'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
'QEMU_IO_OPTIONS', 'QEMU_IO_OPTIONS_NO_FMT',
@@ -98,6 +99,10 @@ def get_env(self) -> Dict[str, str]:
if val is not None:
env[v] = val
+env['PATH'] = os.pathsep.join((
+os.path.join(self.virtual_env, 'bin'),
+os.environ['PATH']
+))
return env
def init_directories(self) -> None:
@@ -107,13 +112,17 @@ def init_directories(self) -> None:
SOCK_DIR
SAMPLE_IMG_DIR
"""
-
-# Path where qemu goodies live in this source tree.
-qemu_srctree_path = Path(__file__, '../../../python').resolve()
+venv_path = Path(self.build_root, 'tests/venv/')
+if not venv_path.exists():
+raise FileNotFoundError(
+f"Virtual environment \"{venv_path!s}\" isn't found."
+" (Maybe you need to run 'make check-venv'"
+" from the build dir?)"
+)
+self.virtual_env: str = str(venv_path)
self.pythonpath = os.pathsep.join(filter(None, (
self.source_iotests,
-str(qemu_srctree_path),
os.getenv('PYTHONPATH'),
)))
@@ -138,7 +147,7 @@ def init_binaries(self) -> None:
PYTHON (for bash tests)
QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, QSD_PROG
"""
-self.python = sys.executable
+self.python: str = os.path.join(self.virtual_env, 'bin', 'python3')
def root(*names: str) -> str:
return os.path.join(self.build_root, *names)
@@ -300,6 +309,7 @@ def print_env(self, prefix: str = '') -> None:
{prefix}GDB_OPTIONS -- {GDB_OPTIONS}
{prefix}VALGRIND_QEMU -- {VALGRIND_QEMU}
{prefix}PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
+{prefix}VIRTUAL_ENV -- {VIRTUAL_ENV}
{prefix}"""
args = collections.defaultdict(str, self.get_env())
--
2.34.1
[RFC PATCH 5/9] tests: use tests/venv to run basevm.py-based scripts
This patch co-opts the virtual environment being used by avocado tests to also run the basevm.py tests. This is being done in preparation for for the qemu.qmp package being removed from qemu.git. As part of the change, remove any sys.path() hacks and treat "qemu" as a normal third-party import. Signed-off-by: John Snow --- tests/vm/Makefile.include | 13 +++-- tests/vm/basevm.py| 6 +++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include index ae91f5043e5..588bc999cc9 100644 --- a/tests/vm/Makefile.include +++ b/tests/vm/Makefile.include @@ -84,10 +84,11 @@ vm-clean-all: $(IMAGES_DIR)/%.img: $(SRC_PATH)/tests/vm/% \ $(SRC_PATH)/tests/vm/basevm.py \ - $(SRC_PATH)/tests/vm/Makefile.include + $(SRC_PATH)/tests/vm/Makefile.include \ + check-venv @mkdir -p $(IMAGES_DIR) $(call quiet-command, \ - $(PYTHON) $< \ + $(TESTS_PYTHON) $< \ $(if $(V)$(DEBUG), --debug) \ $(if $(GENISOIMAGE),--genisoimage $(GENISOIMAGE)) \ $(if $(QEMU_LOCAL),--build-path $(BUILD_DIR)) \ @@ -101,9 +102,9 @@ $(IMAGES_DIR)/%.img:$(SRC_PATH)/tests/vm/% \ # Build in VM $(IMAGE) -vm-build-%: $(IMAGES_DIR)/%.img +vm-build-%: $(IMAGES_DIR)/%.img check-venv $(call quiet-command, \ - $(PYTHON) $(SRC_PATH)/tests/vm/$* \ + $(TESTS_PYTHON) $(SRC_PATH)/tests/vm/$* \ $(if $(V)$(DEBUG), --debug) \ $(if $(DEBUG), --interactive) \ $(if $(J),--jobs $(J)) \ @@ -127,9 +128,9 @@ vm-boot-serial-%: $(IMAGES_DIR)/%.img -device virtio-net-pci,netdev=vnet \ || true -vm-boot-ssh-%: $(IMAGES_DIR)/%.img +vm-boot-ssh-%: $(IMAGES_DIR)/%.img check-venv $(call quiet-command, \ - $(PYTHON) $(SRC_PATH)/tests/vm/$* \ + $(TESTS_PYTHON) $(SRC_PATH)/tests/vm/$* \ $(if $(J),--jobs $(J)) \ $(if $(V)$(DEBUG), --debug) \ $(if $(QEMU_LOCAL),--build-path $(BUILD_DIR)) \ diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 254e11c932b..d7d0413df35 100644 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -18,9 +18,6 @@ import logging import time import datetime -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) -from qemu.machine import QEMUMachine -from qemu.utils import get_info_usernet_hostfwd_port, kvm_available import subprocess import hashlib import argparse @@ -31,6 +28,9 @@ import traceback import shlex +from qemu.machine import QEMUMachine +from qemu.utils import get_info_usernet_hostfwd_port, kvm_available + SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), "..", "keys", "id_rsa") SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__), -- 2.34.1
[RFC PATCH 6/9] tests: add check-venv as a dependency of check and check-block
This patch is being front-loaded before iotests actually relies on the tests/venv being created in order to preserve bisectability. Problems I am aware of here (There are a lot, sorry): - I am not sure the right place to express this dependency, so I did it in tests/Makefile.include. It seems to work. I wasn't sure how to express it in tests/qemu-iotests/meson.build, but I am not sure if that would make it work for both "check" and "check-block" anyway. - I don't really understand why I need empty rules for Make to process the pre-requisite. Without the "do-nothing" recipes, the venv building doesn't seem to trigger at all. What I have seems to work, but I'm worried I broke something unseen. - This adds avocado as a dependency of "check"/"check-block", which is not technically true. It was just a sin of convenience to create one shared "testing venv". Maybe I'll figure out a scheme to have avocado's dependencies be "extras" that get added in to a standard "core set". - This patch ignore the requisite that RPM builds be able to run without internet access, meaning that a PyPI fetch is not permissable. I plan to solve this by using an environment variable (QEMU_CHECK_NO_PYPI) that changes the behavior of the venv setup slightly, and qemu specfiles can opt-in to this behavior. Signed-off-by: John Snow --- tests/Makefile.include | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index dfb678d379f..fa7af711fe5 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -154,10 +154,17 @@ check-acceptance-deprecated-warning: check-acceptance: check-acceptance-deprecated-warning | check-avocado +# Before we delegate to meson, create the python venv for block tests. +.PHONY: check-block +check-block: check-venv + @echo # Without some rule, this doesn't run at all. Why? + + # Consolidated targets .PHONY: check check-clean get-vm-images -check: +check: check-venv + @echo # ??? check-build: run-ninja -- 2.34.1
[RFC PATCH 2/9] tests: add "TESTS_PYTHON" variable to Makefile
This is a convenience feature: $(PYTHON) points to the Python executable we were instructed to use by the configure script. We use that Python to create a virtual environment with the "check-venv" target in tests/Makefile.include. $(TESTS_PYTHON) points to the Python executable belonging to the virtual environment tied to the build. This Python executable is a symlink to the binary used to create the venv, which will be the version provided at configure time. Using $(TESTS_PYTHON) therefore uses the $(PYTHON) executable, but with paths modified to use packages installed to the venv. Signed-off-by: John Snow --- tests/Makefile.include | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index ec84b2ebc04..146aaa96a00 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -89,6 +89,7 @@ TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets))) TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results +TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python ifndef AVOCADO_TESTS AVOCADO_TESTS=tests/avocado endif @@ -108,7 +109,7 @@ $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) $(PYTHON) -m venv $@, \ VENV, $@) $(call quiet-command, \ -$(TESTS_VENV_DIR)/bin/python -m pip -q install -r $(TESTS_VENV_REQ), \ +$(TESTS_PYTHON) -m pip -q install -r $(TESTS_VENV_REQ), \ PIP, $(TESTS_VENV_REQ)) $(call quiet-command, touch $@) @@ -126,7 +127,7 @@ FEDORA_31_DOWNLOAD=$(filter $(FEDORA_31_ARCHES),$(FEDORA_31_ARCHES_CANDIDATES)) # download one specific Fedora 31 image get-vm-image-fedora-31-%: check-venv $(call quiet-command, \ - $(TESTS_VENV_DIR)/bin/python -m avocado vmimage get \ + $(TESTS_PYTHON) -m avocado vmimage get \ --distro=fedora --distro-version=31 --arch=$*, \ "AVOCADO", "Downloading avocado tests VM image for $*") @@ -135,7 +136,7 @@ get-vm-images: check-venv $(patsubst %,get-vm-image-fedora-31-%, $(FEDORA_31_DOW check-avocado: check-venv $(TESTS_RESULTS_DIR) get-vm-images $(call quiet-command, \ -$(TESTS_VENV_DIR)/bin/python -m avocado \ +$(TESTS_PYTHON) -m avocado \ --show=$(AVOCADO_SHOW) run --job-results-dir=$(TESTS_RESULTS_DIR) \ $(if $(AVOCADO_TAGS),, --filter-by-tags-include-empty \ --filter-by-tags-include-empty-key) \ -- 2.34.1
[RFC PATCH 1/9] python: update for mypy 0.950
typeshed (included in mypy) recently updated to improve the typing for WriteTransport objects. I was working around this, but now there's a version where I shouldn't work around it. Unfortunately this creates some minor ugliness if I want to support both pre- and post-0.950 versions. For now, for my sanity, just disable the unused-ignores warning. Signed-off-by: John Snow --- python/qemu/qmp/util.py | 4 +++- python/setup.cfg| 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/python/qemu/qmp/util.py b/python/qemu/qmp/util.py index eaa5fc7d5f9..ca6225e9cda 100644 --- a/python/qemu/qmp/util.py +++ b/python/qemu/qmp/util.py @@ -40,7 +40,9 @@ async def flush(writer: asyncio.StreamWriter) -> None: drain. The flow control limits are restored after the call is completed. """ -transport = cast(asyncio.WriteTransport, writer.transport) +transport = cast( # type: ignore[redundant-cast] +asyncio.WriteTransport, writer.transport +) # https://github.com/python/typeshed/issues/5779 low, high = transport.get_write_buffer_limits() # type: ignore diff --git a/python/setup.cfg b/python/setup.cfg index e877ea56475..c2c61c75190 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -79,6 +79,7 @@ strict = True python_version = 3.6 warn_unused_configs = True namespace_packages = True +warn_unused_ignores = False [mypy-qemu.utils.qom_fuse] # fusepy has no type stubs: -- 2.34.1
[RFC PATCH 0/9] tests: run python tests under the build/tests/venv environment
RFC: This is a very early, crude attempt at switching over to an external Python package dependency for QMP. This series does not actually make the switch in and of itself, but instead just switches to the paradigm of using a venv in general to install the QEMU python packages instead of using PYTHONPATH to load them from the source tree. (By installing the package, we can process dependencies.) I'm sending it to the list so I can show you some of what's ugly so far and my notes on how I might make it less ugly. (1) This doesn't trigger venv creation *from* iotests, it merely prints a friendly error message if "make check-venv" has not been run first. Not the greatest. (2) This isn't acceptable for SRPM builds, because it uses PyPI to fetch packages just-in-time. My thought is to use an environment variable like QEMU_CHECK_NO_INTERNET that changes the behavior of the venv setup process. We can use "--system-site-packages" as an argument to venv creation and "--no-index" as an argument to pip installation to achieve good behavior in SRPM building scenarios. It'd be up to the spec-writer to opt into that behavior. (3) Using one venv for *all* tests means that avocado comes as a pre-req for iotests -- which adds avocado as a BuildRequires for the Fedora SRPM. That's probably not ideal. It may be better to model the test venv as something that can be created in stages: the "core" venv first, and the avocado packages only when needed. You can see in these patches that I wasn't really sure how to tie the check-venv step as a dependency of 'check' or 'check-block', and it winds up feeling kind of hacky and fragile as a result. (Patches 6 and 7 feel particularly fishy.) What I think I would like to do is replace the makefile logic with a Python bootstrapping script. This will allow me to add in environment variable logic to accommodate #2 pretty easily. It will also allow iotests to call into the bootstrap script whenever it detects the venv isn't set up, which it needed to do anyway in order to print a user-friendly error message. Lastly, it will make it easier to create a "tiered" venv that layers in the avocado dependencies only as-needed, which avoids us having to bloat the SRPM build dependencies. In the end, I think that approach will: - Allow us to run iotests without having to run a manual prep step - Keep additional SRPM deps to a minimum - Keep makefile hacks to a minimum The only downside I am really frowning at is that I will have to replicate some "update the venv if it's outdated" logic that is usually handled by the Make system in the venv bootstrapper. Still, I think it's probably the only way to hit all of the requirements here without trying to concoct a fairly complex Makefile. any thoughts? If not, I'll just move on to trying to hack up that version next instead. --js John Snow (9): python: update for mypy 0.950 tests: add "TESTS_PYTHON" variable to Makefile tests: install "qemu" namespace package into venv tests: silence pip upgrade warnings during venv creation tests: use tests/venv to run basevm.py-based scripts tests: add check-venv as a dependency of check and check-block tests: add check-venv to build-tcg-disabled CI recipe iotests: fix source directory location iotests: use tests/venv for running tests .gitlab-ci.d/buildtest.yml | 1 + python/qemu/qmp/util.py| 4 +++- python/setup.cfg | 1 + tests/Makefile.include | 23 +++-- tests/avocado/avocado_qemu/__init__.py | 11 +- tests/avocado/virtio_check_params.py | 1 - tests/avocado/virtio_version.py| 1 - tests/qemu-iotests/testenv.py | 28 +- tests/requirements.txt | 1 + tests/vm/Makefile.include | 13 ++-- tests/vm/basevm.py | 6 +++--- 11 files changed, 57 insertions(+), 33 deletions(-) -- 2.34.1
Re: iotests and python dependencies
On Thu, May 5, 2022 at 5:28 AM Paolo Bonzini wrote: > > On 5/5/22 10:51, Kevin Wolf wrote: > > If not, I guess it would be enough if iotests just checks that the venv > > exists and all of the dependencies are there in the right version and > > error out if not, telling the user to run 'make check-venv'. > > > > Or actually, it could just unconditionally run 'make check-venv' by > > itself, which is probably easier to implement than checking the > > dependencies and more convenient for the user, too. > > Note that you would still have to add a 'check-block: check-venv' > dependency in the Makefile, otherwise two "instances" of check-venv > could run in parallel. > > One small complication is that on BSD systems the binary is actually > called "gmake", so you'd have to pass the variable somehow > > Paolo > Dumb question: where would I express this dependency? I don't know where the top-level "check-block" recipe gets defined. --js
Re: iotests and python dependencies
On Thu, May 5, 2022, 9:16 AM Paolo Bonzini wrote: > On 5/5/22 15:10, John Snow wrote: > > > > > Hm, do we need iotests during an rpm build? Is it because of > > "make check"? > > > > Yes, and this is good, because it prevents us from outputting an > > RPM build that has a broken QEMU in it. > > > > Guess this means I need to make a Fedora package too, though. My hubris. > > I would rather keep python/qemu/qmp as a submodule for a longer time, > and still go through a virtual environment that installs it together > with its pip dependencies. > A small headache relating fixes to both locations, but if you'd like to see it to prove that the installation mechanism works in general, then OK. I'm willing to deal with the pain until the next release to let us go through a testing cycle. Reluctantly. Maybe. I'm assuming you mean as a subpackage and not a [git] submodule. If you do mean git, then ... uh. That might be messy.
Re: iotests and python dependencies
On Thu, May 5, 2022, 8:33 AM Daniel P. Berrangé wrote: > On Thu, May 05, 2022 at 08:08:42AM -0400, John Snow wrote: > > On Thu, May 5, 2022, 4:09 AM Daniel P. Berrangé > wrote: > > > > > On Wed, May 04, 2022 at 03:38:45PM -0400, John Snow wrote: > > > > Howdy! > > > > > > > > So, I want to finally delete python/qemu/qmp from qemu.git, and this > > > > creates a small problem -- namely, iotests needs access to it in > order > > > > to run the python-based tests. > > > > > > > > What I think needs to happen is that we create a virtual environment > > > > that installs python/qemu/. The reason this cannot be done with > > > > PYTHONPATH alone anymore is because the qmp package itself won't be > > > > there anymore, we need an installer like `pip` to actually fetch it > > > > for us and put it somewhere. (i.e., we need to process the > > > > dependencies of python/qemu now and can't treat it as a pre-installed > > > > location.) > > > > > > Having pip fetch it on the fly creates a problem for RPM builds, > > > because the koji build env has no network access. We will, however, > > > have an RPM of python-qemu-qmp installed on the host system though. > > > IOW we need to be able to run iotests using system python and its > > > installed libs, not a virtual env. So if we do anything with a > > > virtual env, it will need to be optional I believe. > > > > > > > Hm, do we need iotests during an rpm build? Is it because of "make > check"? > > Yes, and this is good, because it prevents us from outputting an > RPM build that has a broken QEMU in it. Guess this means I need to make a Fedora package too, though. My hubris. OK, plenty of work to do.
Re: iotests and python dependencies
On Thu, May 5, 2022, 4:09 AM Daniel P. Berrangé wrote: > On Wed, May 04, 2022 at 03:38:45PM -0400, John Snow wrote: > > Howdy! > > > > So, I want to finally delete python/qemu/qmp from qemu.git, and this > > creates a small problem -- namely, iotests needs access to it in order > > to run the python-based tests. > > > > What I think needs to happen is that we create a virtual environment > > that installs python/qemu/. The reason this cannot be done with > > PYTHONPATH alone anymore is because the qmp package itself won't be > > there anymore, we need an installer like `pip` to actually fetch it > > for us and put it somewhere. (i.e., we need to process the > > dependencies of python/qemu now and can't treat it as a pre-installed > > location.) > > Having pip fetch it on the fly creates a problem for RPM builds, > because the koji build env has no network access. We will, however, > have an RPM of python-qemu-qmp installed on the host system though. > IOW we need to be able to run iotests using system python and its > installed libs, not a virtual env. So if we do anything with a > virtual env, it will need to be optional I believe. > Hm, do we need iotests during an rpm build? Is it because of "make check"? It's possible to create a venv and run pip in no-network mode, too. If the package we want is installed on the system or otherwise in pip's cache, it'll succeed without network. If the dependencies require a qemu.qmp that's too new, the pip install will just fail instead. I have to test a way to craft a pip statement that's network *optional* though. i.e. try to fetch and fall back to local otherwise. I think it's worth trying to keep the environment setup code unified, and always use a venv. I think this is tractable, though.
Re: iotests and python dependencies
On Thu, May 5, 2022, 4:51 AM Kevin Wolf wrote: > Am 04.05.2022 um 21:38 hat John Snow geschrieben: > > Howdy! > > > > So, I want to finally delete python/qemu/qmp from qemu.git, and this > > creates a small problem -- namely, iotests needs access to it in order > > to run the python-based tests. > > > > What I think needs to happen is that we create a virtual environment > > that installs python/qemu/. The reason this cannot be done with > > PYTHONPATH alone anymore is because the qmp package itself won't be > > there anymore, we need an installer like `pip` to actually fetch it > > for us and put it somewhere. (i.e., we need to process the > > dependencies of python/qemu now and can't treat it as a pre-installed > > location.) > > > > Avocado tests are already creating a venv for the purposes of > > installing and running Avocado. We can amend e.g. "../../python" to > > tests/requirements.txt and the Avocado environment is A-OK good-to-go. > > The Makefile magic for avocado tests creates a venv-per-build. > > "per build" sounded pretty bad, because I thought it meant building a > new venv every time I run either 'make' or the tests. This would > obviously be very noticable for short-running tests like some iotests. > But fortunately this is not what it does, it keeps the venv around in > the build directory. So it's only per build directory really, which I > guess is fine. > Whoops, yeah. I meant per build directory. In contrast to the Python unit tests themselves, which create some test venvs tied directly to the source directory and are build-agnostic. > > It seems to work well enough. One thing to note here is that the > > supported invocation for avocado tests is only through the Makefile, > > which handles creating and entering the venv to make the command > > seamless. > > > > iotests, however, manages its own execution environment with > > testenv.py, and we support running iotests from outside of the > > Makefile, for example by going to $build/tests/qemu-iotests and > > running ./check. > > Yes, and I agree that this is important. > Figured as much. Not plotting to take this away, I promise. Just getting my requirements straight before I spend time concocting something. > > Now ... I could update testenv.py to be smart enough to create and > > enter a python venv, and have even prototyped this. It seems to work > > pretty well! This approach seemed like the least invasive to how > > iotests are expected to be run and used. But a downside with this > > approach is that now avocado tests and iotests are each managing their > > own python venv. Worse, vm-tests and device-crash-test are still > > unhandled entirely. > > Is there a reason why testenv.py couldn't enter just the shared venv in > build/tests/venv? > It can, but it'd have to be made first so it can enter it. I figured this would only happen "on-demand", like how check-avocado works, so I'd need some way for iotests and the Makefile to share the venv creation code, which is certainly an option. > If not, I guess it would be enough if iotests just checks that the venv > exists and all of the dependencies are there in the right version and > error out if not, telling the user to run 'make check-venv'. > I will spend unreasonable engineering hours avoiding this, if only for pride. I want everything to be as seamless and easy as it's always been. > Or actually, it could just unconditionally run 'make check-venv' by > itself, which is probably easier to implement than checking the > dependencies and more convenient for the user, too. > Oh, that's one way to get them to share the venv-creation pathway. Hadn't occurred to me, but this might be easy to do. > (One more detail: 'make check-venv GIT_SUBMODULES_ACTION=ignore' seems > to make it pretty much instantaneous if the venv is current, but leaving > the submodule mechanism enabled adds a noticable delay.) > Noted. > > I'd like to find a solution where I create a unified python testing > > venv tied to the build shared by avocado, iotests, vm-tests and > > device-crash-test. I'm not completely sure how exactly I'll manage > > that right now, but I wanted to throw this out there in case there are > > some requirements I might be overlooking. > > > > I think vm-tests and avocado-tests can both have a venv created for > > them and activated before the test runs. device-crash-test I believe > > will need a script change in the gitlab ci yaml. iotests is somewhat > > unique in that it needs to run both by ma
iotests and python dependencies
Howdy! So, I want to finally delete python/qemu/qmp from qemu.git, and this creates a small problem -- namely, iotests needs access to it in order to run the python-based tests. What I think needs to happen is that we create a virtual environment that installs python/qemu/. The reason this cannot be done with PYTHONPATH alone anymore is because the qmp package itself won't be there anymore, we need an installer like `pip` to actually fetch it for us and put it somewhere. (i.e., we need to process the dependencies of python/qemu now and can't treat it as a pre-installed location.) Avocado tests are already creating a venv for the purposes of installing and running Avocado. We can amend e.g. "../../python" to tests/requirements.txt and the Avocado environment is A-OK good-to-go. The Makefile magic for avocado tests creates a venv-per-build. It seems to work well enough. One thing to note here is that the supported invocation for avocado tests is only through the Makefile, which handles creating and entering the venv to make the command seamless. iotests, however, manages its own execution environment with testenv.py, and we support running iotests from outside of the Makefile, for example by going to $build/tests/qemu-iotests and running ./check. Now ... I could update testenv.py to be smart enough to create and enter a python venv, and have even prototyped this. It seems to work pretty well! This approach seemed like the least invasive to how iotests are expected to be run and used. But a downside with this approach is that now avocado tests and iotests are each managing their own python venv. Worse, vm-tests and device-crash-test are still unhandled entirely. I'd like to find a solution where I create a unified python testing venv tied to the build shared by avocado, iotests, vm-tests and device-crash-test. I'm not completely sure how exactly I'll manage that right now, but I wanted to throw this out there in case there are some requirements I might be overlooking. I think vm-tests and avocado-tests can both have a venv created for them and activated before the test runs. device-crash-test I believe will need a script change in the gitlab ci yaml. iotests is somewhat unique in that it needs to run both by manual invocation and from makefile invocations. If I want a shared VM between all of these, I'll need to isolate the create-and-enter-venv logic somewhere where it can be shared both inside and outside of a Makefile. I'll see what I can cook up, but if you have any concerns or Cool Ideas, lemme know. I want to make sure this is as painless as I can think to make it. Thanks, --js
Re: [RFC 0/2] introduce QEMUMachind.cmd()
On Fri, Apr 8, 2022 at 1:02 PM Vladimir Sementsov-Ogievskiy
wrote:
>
> Hi all!
>
> I always dreamed about getting rid of pattern
>
> result = self.vm.qmp(...)
> self.assert_qmp(result, 'return', {})
>
> Here is a suggestion to switch to
>
> self.vm.cmd(...)
>
> pattern instead.
Yeah, I am absolutely on board for this!
>
> I'm not sure we really want to update so many tests. May be just commit
> patch 01, and use new interface for new code. On the other hand, old
> code always used as an example to write the new one.
I think it's worth updating all the old tests ... especially if you've
already done it here. We could even do something like what I did with
qemu_img() and qemu_io() and have the uncaught exception print a bunch
of information to the screen to help make it extremely obvious as to
what failed and why.
If you can rebase this, I'd love to review it more carefully - it
aligns with my own selfish goals and interests :) The Python branch
was merged recently and so we should be all set.
>
> The series is based on John's python branch.
>
> Vladimir Sementsov-Ogievskiy (2):
> python/machine.py: upgrade vm.command() method
> iotests: use vm.cmd() instead of vm.qmp() where appropriate
>
> python/qemu/machine/machine.py| 16 +-
> tests/qemu-iotests/030| 168 +++
> tests/qemu-iotests/040| 167 +++---
> tests/qemu-iotests/041| 474 --
> tests/qemu-iotests/045| 15 +-
> tests/qemu-iotests/055| 61 +--
> tests/qemu-iotests/056| 23 +-
> tests/qemu-iotests/093| 41 +-
> tests/qemu-iotests/118| 221
> tests/qemu-iotests/124| 69 ++-
> tests/qemu-iotests/129| 13 +-
> tests/qemu-iotests/132| 5 +-
> tests/qemu-iotests/139| 43 +-
> tests/qemu-iotests/147| 30 +-
> tests/qemu-iotests/151| 40 +-
> tests/qemu-iotests/155| 53 +-
> tests/qemu-iotests/165| 7 +-
> tests/qemu-iotests/196| 3 +-
> tests/qemu-iotests/205| 6 +-
> tests/qemu-iotests/245| 245 -
> tests/qemu-iotests/256| 34 +-
> tests/qemu-iotests/257| 36 +-
> tests/qemu-iotests/264| 31 +-
> tests/qemu-iotests/281| 21 +-
> tests/qemu-iotests/295| 27 +-
> tests/qemu-iotests/296| 14 +-
> tests/qemu-iotests/298| 13 +-
> tests/qemu-iotests/300| 50 +-
> tests/qemu-iotests/iotests.py | 6 +-
> .../tests/migrate-bitmaps-postcopy-test | 31 +-
> tests/qemu-iotests/tests/migrate-bitmaps-test | 37 +-
> .../qemu-iotests/tests/migrate-during-backup | 40 +-
> .../qemu-iotests/tests/migration-permissions | 9 +-
> tests/qemu-iotests/tests/mirror-top-perms | 15 +-
> 34 files changed, 821 insertions(+), 1243 deletions(-)
Is there anything missing, to your knowledge?
--js
Re: [PATCH v3 00/12] iotests: add enhanced debugging info to qemu-io failures
On Mon, Apr 25, 2022 at 8:31 AM Hanna Reitz wrote: > > On 18.04.22 23:14, John Snow wrote: > > GitLab: https://gitlab.com/jsnow/qemu/-/commits/iotests_qemu_io_diagnostics > > > > Howdy, > > > > This series does for qemu_io() what we've done for qemu_img() and makes > > it a function that checks the return code by default and raises an > > Exception when things do not go according to plan. > > > > This series removes qemu_io_pipe_and_status(), qemu_io_silent(), and > > qemu_io_silent_check() in favor of just qemu_io(). > > > > V3: > > > > - Rebased > > - Squashed the patches that I said I would > > Thanks, applied to my block branch: > > https://gitlab.com/hreitz/qemu/-/commits/block > > Hanna > Thanks! Please pester me if something comes up as a result of these patches. :) --js
[PULL 15/17] python: rename qemu.aqmp to qemu.qmp
Now that we are fully switched over to the new QMP library, move it back
over the old namespace. This is being done primarily so that we may
upload this package simply as "qemu.qmp" without introducing confusion
over whether or not "aqmp" is a new protocol or not.
The trade-off is increased confusion inside the QEMU developer
tree. Sorry!
Note: the 'private' member "_aqmp" in legacy.py also changes to "_qmp";
not out of necessity, but just to remove any traces of the "aqmp"
name.
Signed-off-by: John Snow
Reviewed-by: Beraldo Leal
Acked-by: Hanna Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-id: 20220330172812.3427355-8-js...@redhat.com
Signed-off-by: John Snow
---
python/PACKAGE.rst| 4 +--
python/README.rst | 4 +--
python/qemu/machine/machine.py| 4 +--
python/qemu/machine/qtest.py | 2 +-
python/qemu/{aqmp => qmp}/__init__.py | 6 ++--
python/qemu/{aqmp => qmp}/aqmp_tui.py | 0
python/qemu/{aqmp => qmp}/error.py| 0
python/qemu/{aqmp => qmp}/events.py | 2 +-
python/qemu/{aqmp => qmp}/legacy.py | 38 +++
python/qemu/{aqmp => qmp}/message.py | 0
python/qemu/{aqmp => qmp}/models.py | 0
python/qemu/{aqmp => qmp}/protocol.py | 4 +--
python/qemu/{aqmp => qmp}/py.typed| 0
python/qemu/{aqmp => qmp}/qmp_client.py | 16 +-
python/qemu/{aqmp => qmp}/qmp_shell.py| 4 +--
python/qemu/{aqmp => qmp}/util.py | 0
python/qemu/utils/qemu_ga_client.py | 4 +--
python/qemu/utils/qom.py | 2 +-
python/qemu/utils/qom_common.py | 4 +--
python/qemu/utils/qom_fuse.py | 2 +-
python/setup.cfg | 10 +++---
python/tests/protocol.py | 14 -
scripts/cpu-x86-uarch-abi.py | 2 +-
scripts/device-crash-test | 4 +--
scripts/qmp/qmp-shell | 2 +-
scripts/qmp/qmp-shell-wrap| 2 +-
scripts/render_block_graph.py | 4 +--
scripts/simplebench/bench_block_job.py| 2 +-
tests/qemu-iotests/iotests.py | 2 +-
tests/qemu-iotests/tests/mirror-top-perms | 4 +--
30 files changed, 71 insertions(+), 71 deletions(-)
rename python/qemu/{aqmp => qmp}/__init__.py (87%)
rename python/qemu/{aqmp => qmp}/aqmp_tui.py (100%)
rename python/qemu/{aqmp => qmp}/error.py (100%)
rename python/qemu/{aqmp => qmp}/events.py (99%)
rename python/qemu/{aqmp => qmp}/legacy.py (91%)
rename python/qemu/{aqmp => qmp}/message.py (100%)
rename python/qemu/{aqmp => qmp}/models.py (100%)
rename python/qemu/{aqmp => qmp}/protocol.py (99%)
rename python/qemu/{aqmp => qmp}/py.typed (100%)
rename python/qemu/{aqmp => qmp}/qmp_client.py (97%)
rename python/qemu/{aqmp => qmp}/qmp_shell.py (99%)
rename python/qemu/{aqmp => qmp}/util.py (100%)
diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index ddfa9ba3f59..b0b86cc4c31 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
Usage
-
-The ``qemu.aqmp`` subpackage provides a library for communicating with
+The ``qemu.qmp`` subpackage provides a library for communicating with
QMP servers. The ``qemu.machine`` subpackage offers rudimentary
facilities for launching and managing QEMU processes. Refer to each
package's documentation
-(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
for more information.
Contributing
diff --git a/python/README.rst b/python/README.rst
index eb5213337d2..9c1fceaee73 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
This directory houses Python tooling used by the QEMU project to build,
configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
``setup.py`` is used by ``pip`` to install this tooling to the current
environment. ``setup.cfg`` provides the packaging configuration used by
@@ -59,7 +59,7 @@ Package installation also normally provides executable
console scripts,
so that tools like ``qmp-shell`` are always available via $PATH. To
invoke them without installation, you can invoke e.g.:
-``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
+``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
The mappings between console script name and python module path can be
found in ``setup.cfg``.
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 41be025ac7b..07ac5a710be 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,8 +40,8
[PULL 16/17] python: rename 'aqmp-tui' to 'qmp-tui'
This is the last vestige of the "aqmp" moniker surviving in the tree; remove it.
Signed-off-by: John Snow
Reviewed-by: Beraldo Leal
Reviewed-by: Vladimir Sementsov-Ogievskiy
Message-id: 20220330172812.3427355-9-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/qmp/{aqmp_tui.py => qmp_tui.py} | 12 ++--
python/setup.cfg| 6 +++---
2 files changed, 9 insertions(+), 9 deletions(-)
rename python/qemu/qmp/{aqmp_tui.py => qmp_tui.py} (98%)
diff --git a/python/qemu/qmp/aqmp_tui.py b/python/qemu/qmp/qmp_tui.py
similarity index 98%
rename from python/qemu/qmp/aqmp_tui.py
rename to python/qemu/qmp/qmp_tui.py
index 59d3036be38..ce239d8979b 100644
--- a/python/qemu/qmp/aqmp_tui.py
+++ b/python/qemu/qmp/qmp_tui.py
@@ -6,13 +6,13 @@
# This work is licensed under the terms of the GNU LGPL, version 2 or
# later. See the COPYING file in the top-level directory.
"""
-AQMP TUI
+QMP TUI
-AQMP TUI is an asynchronous interface built on top the of the AQMP library.
+QMP TUI is an asynchronous interface built on top the of the QMP library.
It is the successor of QMP-shell and is bought-in as a replacement for it.
-Example Usage: aqmp-tui
-Full Usage: aqmp-tui --help
+Example Usage: qmp-tui
+Full Usage: qmp-tui --help
"""
import argparse
@@ -129,7 +129,7 @@ def has_handler_type(logger: logging.Logger,
class App(QMPClient):
"""
-Implements the AQMP TUI.
+Implements the QMP TUI.
Initializes the widgets and starts the urwid event loop.
@@ -612,7 +612,7 @@ def main() -> None:
Driver of the whole script, parses arguments, initialize the TUI and
the logger.
"""
-parser = argparse.ArgumentParser(description='AQMP TUI')
+parser = argparse.ArgumentParser(description='QMP TUI')
parser.add_argument('qmp_server', help='Address of the QMP server. '
'Format ')
parser.add_argument('--num-retries', type=int, default=10,
diff --git a/python/setup.cfg b/python/setup.cfg
index 773e51b34e7..e877ea56475 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -51,7 +51,7 @@ devel =
fuse =
fusepy >= 2.0.4
-# AQMP TUI dependencies
+# QMP TUI dependencies
tui =
urwid >= 2.1.2
urwid-readline >= 0.13
@@ -68,7 +68,7 @@ console_scripts =
qemu-ga-client = qemu.utils.qemu_ga_client:main
qmp-shell = qemu.qmp.qmp_shell:main
qmp-shell-wrap = qemu.qmp.qmp_shell:main_wrap
-aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
+qmp-tui = qemu.qmp.qmp_tui:main [tui]
[flake8]
extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's
@@ -84,7 +84,7 @@ namespace_packages = True
# fusepy has no type stubs:
allow_subclassing_any = True
-[mypy-qemu.qmp.aqmp_tui]
+[mypy-qemu.qmp.qmp_tui]
# urwid and urwid_readline have no type stubs:
allow_subclassing_any = True
--
2.34.1
[PULL 14/17] python: re-enable pylint duplicate-code warnings
With the old library gone, there's nothing duplicated in the tree, so the warning suppression can be removed. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Beraldo Leal Message-id: 20220330172812.3427355-7-js...@redhat.com Signed-off-by: John Snow --- python/setup.cfg | 1 - 1 file changed, 1 deletion(-) diff --git a/python/setup.cfg b/python/setup.cfg index 4340c29a24f..49e3c285f19 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -118,7 +118,6 @@ disable=consider-using-f-string, too-many-function-args, # mypy handles this with less false positives. too-many-instance-attributes, no-member, # mypy also handles this better. -duplicate-code, # To be removed by the end of this patch series. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.34.1
[PULL 12/17] python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
Copy the docstrings out of qemu.qmp, adjusting them as necessary to
more accurately reflect the current state of this class.
(Licensing: This is copying and modifying GPLv2-only licensed docstrings
into a GPLv2-only file.)
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
Message-id: 20220330172812.3427355-5-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 98 ++
1 file changed, 90 insertions(+), 8 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 10c7c99c4f0..dfcd20bbd23 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -1,7 +1,13 @@
"""
-Sync QMP Wrapper
+(Legacy) Sync QMP Wrapper
-This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+This module provides the `QEMUMonitorProtocol` class, which is a
+synchronous wrapper around `QMPClient`.
+
+Its design closely resembles that of the original QEMUMonitorProtocol
+class, originally written by Luiz Capitulino. It is provided here for
+compatibility with scripts inside the QEMU source tree that expect the
+old interface.
"""
#
@@ -50,9 +56,6 @@
# {} is the QMPReturnValue.
-# pylint: disable=missing-docstring
-
-
class QMPBadPortError(QMPError):
"""
Unable to parse socket address: Port was non-numerical.
@@ -60,6 +63,17 @@ class QMPBadPortError(QMPError):
class QEMUMonitorProtocol:
+"""
+Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP)
+and then allow to handle commands and events.
+
+:param address: QEMU address, can be either a unix socket path (string)
+ or a tuple in the form ( address, port ) for a TCP
+ connection
+:param server: Act as the socket server. (See 'accept')
+:param nickname: Optional nickname used for logging.
+"""
+
def __init__(self, address: SocketAddrT,
server: bool = False,
nickname: Optional[str] = None):
@@ -121,6 +135,12 @@ def parse_address(cls, address: str) -> SocketAddrT:
return address
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+"""
+Connect to the QMP Monitor and perform capabilities negotiation.
+
+:return: QMP greeting dict, or None if negotiate is false
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = negotiate
self._aqmp.negotiate = negotiate
@@ -130,6 +150,16 @@ def connect(self, negotiate: bool = True) ->
Optional[QMPMessage]:
return self._get_greeting()
def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+"""
+Await connection from QMP Monitor and perform capabilities negotiation.
+
+:param timeout:
+timeout in seconds (nonnegative float number, or None).
+If None, there is no timeout, and this may block forever.
+
+:return: QMP greeting dict
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = True
self._aqmp.negotiate = True
@@ -140,6 +170,12 @@ def accept(self, timeout: Optional[float] = 15.0) ->
QMPMessage:
return ret
def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+"""
+Send a QMP command to the QMP Monitor.
+
+:param qmp_cmd: QMP command to be sent as a Python dict
+:return: QMP response as a Python dict
+"""
return dict(
self._sync(
# pylint: disable=protected-access
@@ -158,9 +194,9 @@ def cmd(self, name: str,
"""
Build a QMP command and send it to the QMP Monitor.
-@param name: command name (string)
-@param args: command arguments (dict)
-@param cmd_id: command id (dict, list, string or int)
+:param name: command name (string)
+:param args: command arguments (dict)
+:param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd: QMPMessage = {'execute': name}
if args:
@@ -170,6 +206,9 @@ def cmd(self, name: str,
return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
return self._sync(
self._aqmp.execute(cmd, kwds),
self._timeout
@@ -177,6 +216,19 @@ def command(self, cmd: str, **kwds: object) ->
QMPReturnValue:
def pull_event(self,
wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+"""
+Pulls a sing
[PULL 06/17] python/aqmp: relicense as LGPLv2+
I am the sole author of all of the async QMP code (python/qemu/aqmp) with the following exceptions: python/qemu/aqmp/qmp_shell.py and python/qemu/aqmp/legacy.py were written by Luiz Capitulino (et al) and are already licensed separately as GPLv2 (only). aqmp_tui.py was written by Niteesh Babu G S and is licensed as GPLv2+. I wish to relicense as LGPLv2+ in order to provide as much flexibility as I reasonably can, while retaining a copyleft license. It is my belief that LGPLv2+ is a suitable license for the Python ecosystem that aligns with the goals and philosophy of the QEMU project. The intent is to eventually drop legacy.py, leaving only library code that is LGPLv2+. Signed-off-by: John Snow Message-id: 20220325200438.2556381-3-js...@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py index 4c22c380790..2b69b264f4f 100644 --- a/python/qemu/aqmp/__init__.py +++ b/python/qemu/aqmp/__init__.py @@ -11,15 +11,15 @@ managing QMP events. """ -# Copyright (C) 2020, 2021 John Snow for Red Hat, Inc. +# Copyright (C) 2020-2022 John Snow for Red Hat, Inc. # # Authors: # John Snow # # Based on earlier work by Luiz Capitulino . # -# This work is licensed under the terms of the GNU GPL, version 2. See -# the COPYING file in the top-level directory. +# This work is licensed under the terms of the GNU LGPL, version 2 or +# later. See the COPYING file in the top-level directory. import logging -- 2.34.1
[PULL 04/17] iotests: switch to AQMP
iotests is already using async QMP, but to finalize the switchover we only need to update any remaining import paths to rely solely on the new library instead. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Beraldo Leal Acked-by: Hanna Reitz Message-id: 20220321203315.909411-5-js...@redhat.com [Fixed minor rebase conflict. --js] Signed-off-by: John Snow --- tests/qemu-iotests/iotests.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index fe10a6cf05a..6f9aa38e0e8 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -37,9 +37,8 @@ from contextlib import contextmanager -from qemu.aqmp.legacy import QEMUMonitorProtocol +from qemu.aqmp.legacy import QMPMessage, QEMUMonitorProtocol from qemu.machine import qtest -from qemu.qmp import QMPMessage from qemu.utils import VerboseProcessError # Use this logger for logging messages directly from the iotests module -- 2.34.1
[PULL 01/17] python/machine: permanently switch to AQMP
Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
switch from sync qmp to async qmp permanent. Update exceptions and
import paths as necessary.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
Acked-by: Hanna Reitz
Message-id: 20220321203315.909411-2-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 18 +++---
python/qemu/machine/qtest.py | 2 +-
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a5972fab4d2..41be025ac7b 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,21 +40,16 @@
TypeVar,
)
-from qemu.qmp import ( # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
+from qemu.aqmp.legacy import (
+QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
-SocketAddrT,
)
from . import console_socket
-if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
-from qemu.qmp import QEMUMonitorProtocol
-else:
-from qemu.aqmp.legacy import QEMUMonitorProtocol
-
-
LOG = logging.getLogger(__name__)
@@ -743,8 +738,9 @@ def events_wait(self,
:param timeout: Optional timeout, in seconds.
See QEMUMonitorProtocol.pull_event.
-:raise QMPTimeoutError: If timeout was non-zero and no matching events
-were found.
+:raise asyncio.TimeoutError:
+If timeout was non-zero and no matching events were found.
+
:return: A QMP event matching the filter criteria.
If timeout was 0 and no event matched, None.
"""
@@ -767,7 +763,7 @@ def _match(event: QMPMessage) -> bool:
event = self._qmp.pull_event(wait=timeout)
if event is None:
# NB: None is only returned when timeout is false-ish.
-# Timeouts raise QMPTimeoutError instead!
+# Timeouts raise asyncio.TimeoutError instead!
break
if _match(event):
return event
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index f2f9aaa5e50..13e0aaff846 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
TextIO,
)
-from qemu.qmp import SocketAddrT # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
from .machine import QEMUMachine
--
2.34.1
[PULL 17/17] python/qmp: remove pylint workaround from legacy.py
Pylint upgraded recently (2.13.z) and having a pylint: disable comment in the middle of an argument field causes it some grief (It appears to stop parsing when it encounters it, causing some syntax problems). Since the duplicate line threshold was bumped up in 22305c2a081b, we don't need this workaround anymore. Drop it. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20220330172812.3427355-10-js...@redhat.com Signed-off-by: John Snow --- python/qemu/qmp/legacy.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py index a8629b44dff..03b5574618f 100644 --- a/python/qemu/qmp/legacy.py +++ b/python/qemu/qmp/legacy.py @@ -106,8 +106,6 @@ def __enter__(self: _T) -> _T: return self def __exit__(self, - # pylint: disable=duplicate-code - # see https://github.com/PyCQA/pylint/issues/3619 exc_type: Optional[Type[BaseException]], exc_val: Optional[BaseException], exc_tb: Optional[TracebackType]) -> None: -- 2.34.1
[PULL 11/17] python/aqmp: fully separate from qmp.QEMUMonitorProtocol
After this patch, qemu.aqmp.legacy.QEMUMonitorProtocol no longer
inherits from qemu.qmp.QEMUMonitorProtocol. To do this, several
inherited methods need to be explicitly re-defined.
(Licensing: This is copying and modifying GPLv2-only code into a
GPLv2-only file.)
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
Message-id: 20220330172812.3427355-4-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 37 +++--
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index f0262749491..10c7c99c4f0 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -16,18 +16,18 @@
#
import asyncio
+from types import TracebackType
from typing import (
Any,
Awaitable,
Dict,
List,
Optional,
+Type,
TypeVar,
Union,
)
-import qemu.qmp
-
from .error import QMPError
from .protocol import Runstate, SocketAddrT
from .qmp_client import QMPClient
@@ -59,12 +59,11 @@ class QMPBadPortError(QMPError):
"""
-class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+class QEMUMonitorProtocol:
def __init__(self, address: SocketAddrT,
server: bool = False,
nickname: Optional[str] = None):
-# pylint: disable=super-init-not-called
self._aqmp = QMPClient(nickname)
self._aloop = asyncio.get_event_loop()
self._address = address
@@ -88,7 +87,18 @@ def _get_greeting(self) -> Optional[QMPMessage]:
return self._aqmp.greeting._asdict()
return None
-# __enter__ and __exit__ need no changes
+def __enter__(self: _T) -> _T:
+# Implement context manager enter function.
+return self
+
+def __exit__(self,
+ # pylint: disable=duplicate-code
+ # see https://github.com/PyCQA/pylint/issues/3619
+ exc_type: Optional[Type[BaseException]],
+ exc_val: Optional[BaseException],
+ exc_tb: Optional[TracebackType]) -> None:
+# Implement context manager exit function.
+self.close()
@classmethod
def parse_address(cls, address: str) -> SocketAddrT:
@@ -142,7 +152,22 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
)
)
-# Default impl of cmd() delegates to cmd_obj
+def cmd(self, name: str,
+args: Optional[Dict[str, object]] = None,
+cmd_id: Optional[object] = None) -> QMPMessage:
+"""
+Build a QMP command and send it to the QMP Monitor.
+
+@param name: command name (string)
+@param args: command arguments (dict)
+@param cmd_id: command id (dict, list, string or int)
+"""
+qmp_cmd: QMPMessage = {'execute': name}
+if args:
+qmp_cmd['arguments'] = args
+if cmd_id:
+qmp_cmd['id'] = cmd_id
+return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
return self._sync(
--
2.34.1
[PULL 13/17] python: remove the old QMP package
Thank you for your service!
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
Message-id: 20220330172812.3427355-6-js...@redhat.com
Signed-off-by: John Snow
---
python/PACKAGE.rst | 4 +-
python/README.rst | 2 +-
python/qemu/qmp/README.rst | 9 -
python/qemu/qmp/__init__.py | 396
python/qemu/qmp/py.typed| 0
python/setup.cfg| 3 +-
6 files changed, 4 insertions(+), 410 deletions(-)
delete mode 100644 python/qemu/qmp/README.rst
delete mode 100644 python/qemu/qmp/__init__.py
delete mode 100644 python/qemu/qmp/py.typed
diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index b0b86cc4c31..ddfa9ba3f59 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
Usage
-
-The ``qemu.qmp`` subpackage provides a library for communicating with
+The ``qemu.aqmp`` subpackage provides a library for communicating with
QMP servers. The ``qemu.machine`` subpackage offers rudimentary
facilities for launching and managing QEMU processes. Refer to each
package's documentation
-(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
for more information.
Contributing
diff --git a/python/README.rst b/python/README.rst
index fcf74f69eae..eb5213337d2 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
This directory houses Python tooling used by the QEMU project to build,
configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
``setup.py`` is used by ``pip`` to install this tooling to the current
environment. ``setup.cfg`` provides the packaging configuration used by
diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
deleted file mode 100644
index 5bfb82535f8..000
--- a/python/qemu/qmp/README.rst
+++ /dev/null
@@ -1,9 +0,0 @@
-qemu.qmp package
-
-
-This package provides a library used for connecting to and communicating
-with QMP servers. It is used extensively by iotests, vm tests,
-avocado tests, and other utilities in the ./scripts directory. It is
-not a fully-fledged SDK and is subject to change at any time.
-
-See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
deleted file mode 100644
index 4e086411544..000
--- a/python/qemu/qmp/__init__.py
+++ /dev/null
@@ -1,396 +0,0 @@
-"""
-QEMU Monitor Protocol (QMP) development library & tooling.
-
-This package provides a fairly low-level class for communicating to QMP
-protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
-QEMU Storage Daemon. This library is not intended for production use.
-
-`QEMUMonitorProtocol` is the primary class of interest, and all errors
-raised derive from `QMPError`.
-"""
-
-# Copyright (C) 2009, 2010 Red Hat Inc.
-#
-# Authors:
-# Luiz Capitulino
-#
-# This work is licensed under the terms of the GNU GPL, version 2. See
-# the COPYING file in the top-level directory.
-
-import errno
-import json
-import logging
-import socket
-import struct
-from types import TracebackType
-from typing import (
-Any,
-Dict,
-List,
-Optional,
-TextIO,
-Tuple,
-Type,
-TypeVar,
-Union,
-cast,
-)
-
-
-#: QMPMessage is an entire QMP message of any kind.
-QMPMessage = Dict[str, Any]
-
-#: QMPReturnValue is the 'return' value of a command.
-QMPReturnValue = object
-
-#: QMPObject is any object in a QMP message.
-QMPObject = Dict[str, object]
-
-# QMPMessage can be outgoing commands or incoming events/returns.
-# QMPReturnValue is usually a dict/json object, but due to QAPI's
-# 'returns-whitelist', it can actually be anything.
-#
-# {'return': {}} is a QMPMessage,
-# {} is the QMPReturnValue.
-
-
-InternetAddrT = Tuple[str, int]
-UnixAddrT = str
-SocketAddrT = Union[InternetAddrT, UnixAddrT]
-
-
-class QMPError(Exception):
-"""
-QMP base exception
-"""
-
-
-class QMPConnectError(QMPError):
-"""
-QMP connection exception
-"""
-
-
-class QMPCapabilitiesError(QMPError):
-"""
-QMP negotiate capabilities exception
-"""
-
-
-class QMPTimeoutError(QMPError):
-"""
-QMP timeout exception
-"""
-
-
-class QMPProtocolError(QMPError):
-"""
-QMP protocol error; unexpected response
-"""
-
-
-class QMPResponseError(QMPError):
-"""
-Represents erroneous QMP monitor reply
-"""
-def __init__(self, reply: QMPMessage):
[PULL 10/17] python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
Shift these definitions over from the qmp package to the async qmp
package.
(Licensing: this is a lateral move, from GPLv2 (only) to GPLv2 (only))
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
Message-id: 20220330172812.3427355-3-js...@redhat.com
Signed-off-by: John Snow
---
python/qemu/aqmp/aqmp_tui.py | 3 +--
python/qemu/aqmp/legacy.py | 30 ++
python/qemu/qmp/__init__.py | 26 --
3 files changed, 27 insertions(+), 32 deletions(-)
diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index 946ba9af24e..59d3036be38 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -35,9 +35,8 @@
import urwid
import urwid_readline
-from qemu.qmp import QEMUMonitorProtocol, QMPBadPortError
-
from .error import ProtocolError
+from .legacy import QEMUMonitorProtocol, QMPBadPortError
from .message import DeserializationError, Message, UnexpectedTypeError
from .protocol import ConnectError, Runstate
from .qmp_client import ExecInterruptedError, QMPClient
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index f86cb298049..f0262749491 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -33,9 +33,6 @@
from .qmp_client import QMPClient
-# (Temporarily) Re-export QMPBadPortError
-QMPBadPortError = qemu.qmp.QMPBadPortError
-
#: QMPMessage is an entire QMP message of any kind.
QMPMessage = Dict[str, Any]
@@ -56,6 +53,12 @@
# pylint: disable=missing-docstring
+class QMPBadPortError(QMPError):
+"""
+Unable to parse socket address: Port was non-numerical.
+"""
+
+
class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
def __init__(self, address: SocketAddrT,
server: bool = False,
@@ -86,7 +89,26 @@ def _get_greeting(self) -> Optional[QMPMessage]:
return None
# __enter__ and __exit__ need no changes
-# parse_address needs no changes
+
+@classmethod
+def parse_address(cls, address: str) -> SocketAddrT:
+"""
+Parse a string into a QMP address.
+
+Figure out if the argument is in the port:host form.
+If it's not, it's probably a file path.
+"""
+components = address.split(':')
+if len(components) == 2:
+try:
+port = int(components[1])
+except ValueError:
+msg = f"Bad port: '{components[1]}' in '{address}'."
+raise QMPBadPortError(msg) from None
+return (components[0], port)
+
+# Treat as filepath.
+return address
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
self._aqmp.await_greeting = negotiate
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 358c0971d06..4e086411544 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -102,12 +102,6 @@ def __init__(self, reply: QMPMessage):
self.reply = reply
-class QMPBadPortError(QMPError):
-"""
-Unable to parse socket address: Port was non-numerical.
-"""
-
-
class QEMUMonitorProtocol:
"""
Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
@@ -237,26 +231,6 @@ def __exit__(self,
# Implement context manager exit function.
self.close()
-@classmethod
-def parse_address(cls, address: str) -> SocketAddrT:
-"""
-Parse a string into a QMP address.
-
-Figure out if the argument is in the port:host form.
-If it's not, it's probably a file path.
-"""
-components = address.split(':')
-if len(components) == 2:
-try:
-port = int(components[1])
-except ValueError:
-msg = f"Bad port: '{components[1]}' in '{address}'."
-raise QMPBadPortError(msg) from None
-return (components[0], port)
-
-# Treat as filepath.
-return address
-
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
"""
Connect to the QMP Monitor and perform capabilities negotiation.
--
2.34.1
[PULL 08/17] python/aqmp-tui: relicense as LGPLv2+
aqmp-tui, the async QMP text user interface tool, is presently licensed as GPLv2+. I intend to include this tool as an add-on to an LGPLv2+ library package hosted on PyPI.org. I've selected LGPLv2+ to maximize compatibility with other licenses while retaining a copyleft license. To keep licensing matters simple, I'd like to relicense this tool as LGPLv2+ as well in order to keep the resultant license of the hosted release files simple -- even if library users won't "link against" this command line tool. Therefore, I am asking permission to loosen the license. Niteesh is effectively the sole author of this code, with scattered lines from myself. Signed-off-by: John Snow Reviewed-by: G S Niteesh Babu Message-id: 20220325200438.2556381-5-js...@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/aqmp_tui.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py index f1e926dd756..946ba9af24e 100644 --- a/python/qemu/aqmp/aqmp_tui.py +++ b/python/qemu/aqmp/aqmp_tui.py @@ -3,7 +3,7 @@ # Authors: # Niteesh Babu G S # -# This work is licensed under the terms of the GNU GPL, version 2 or +# This work is licensed under the terms of the GNU LGPL, version 2 or # later. See the COPYING file in the top-level directory. """ AQMP TUI -- 2.34.1
[PULL 09/17] python: temporarily silence pylint duplicate-code warnings
The next several commits copy some code from qemu.qmp to qemu.aqmp, then delete qemu.qmp. In the interim, to prevent test failures, the duplicate code detection needs to be silenced to prevent bisect problems with CI testing. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20220330172812.3427355-2-js...@redhat.com Signed-off-by: John Snow --- python/setup.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/python/setup.cfg b/python/setup.cfg index 241f243e8b9..cdeced44d2b 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -119,6 +119,7 @@ disable=consider-using-f-string, too-many-function-args, # mypy handles this with less false positives. too-many-instance-attributes, no-member, # mypy also handles this better. +duplicate-code, # To be removed by the end of this patch series. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.34.1
[PULL 03/17] iotests/mirror-top-perms: switch to AQMP
We don't have to maintain compatibility with both QMP libraries anymore,
so we can just remove the old exception. While we're here, take
advantage of the extra fields present in the VMLaunchFailure exception
that machine.py now raises.
(Note: I'm leaving the logging suppression here unchanged. I had
suggested previously we use filters to scrub the PID out of the logging
information so it could just be diffed as part of the iotest output, but
that meant *always* scrubbing PID from logger output, which defeated the
point of even offering that information in the output to begin with.
Ultimately, I decided it's fine to just suppress the logger temporarily.)
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Acked-by: Hanna Reitz
Message-id: 20220321203315.909411-4-js...@redhat.com
Signed-off-by: John Snow
---
tests/qemu-iotests/tests/mirror-top-perms | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 6ac8d5efccb..a9f275cd7f2 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -22,7 +22,6 @@
import os
from qemu.machine import machine
-from qemu.qmp import QMPConnectError
import iotests
from iotests import change_log_level, qemu_img
@@ -98,15 +97,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
-# Silence AQMP errors temporarily.
-# TODO: Remove this and just allow the errors to be logged when
-# AQMP fully replaces QMP.
+# Silence AQMP logging errors temporarily.
with change_log_level('qemu.aqmp'):
self.vm_b.launch()
print('ERROR: VM B launched successfully, '
'this should not have happened')
-except (QMPConnectError, machine.VMLaunchFailure):
-assert 'Is another process using the image' in self.vm_b.get_log()
+except machine.VMLaunchFailure as exc:
+assert 'Is another process using the image' in exc.output
result = self.vm.qmp('block-job-cancel',
device='mirror')
--
2.34.1
[PULL 05/17] python/aqmp: add explicit GPLv2 license to legacy.py
The legacy.py module is heavily based on the QMP module by Luiz Capitulino (et al) which is licensed as explicit GPLv2-only. The async QMP package is currently licensed similarly, but I intend to relicense the async package to the more flexible LGPLv2+. In preparation for that change, make the license on legacy.py explicit. Signed-off-by: John Snow Message-id: 20220325200438.2556381-2-js...@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/legacy.py | 11 +++ 1 file changed, 11 insertions(+) diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py index 46026e9fdc6..f86cb298049 100644 --- a/python/qemu/aqmp/legacy.py +++ b/python/qemu/aqmp/legacy.py @@ -4,6 +4,17 @@ This class pretends to be qemu.qmp.QEMUMonitorProtocol. """ +# +# Copyright (C) 2009-2022 Red Hat Inc. +# +# Authors: +# Luiz Capitulino +# John Snow +# +# This work is licensed under the terms of the GNU GPL, version 2. See +# the COPYING file in the top-level directory. +# + import asyncio from typing import ( Any, -- 2.34.1
[PULL 02/17] scripts/bench-block-job: switch to AQMP
For this commit, we only need to remove accommodations for the
synchronous QMP library.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
Acked-by: Hanna Reitz
Message-id: 20220321203315.909411-3-js...@redhat.com
Signed-off-by: John Snow
---
scripts/simplebench/bench_block_job.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index a403c35b08f..af9d1646a46 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -27,7 +27,6 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
-from qemu.qmp import QMPConnectError
from qemu.aqmp import ConnectError
@@ -50,7 +49,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, ConnectError, socket.timeout):
+except (ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
--
2.34.1
[PULL 07/17] python/qmp-shell: relicense as LGPLv2+
qmp-shell is presently licensed as GPLv2 (only). I intend to include this tool as an add-on to an LGPLv2+ library package hosted on PyPI.org. I've selected LGPLv2+ to maximize compatibility with other licenses while retaining a copyleft license. To keep licensing matters simple, I'd like to relicense this tool as LGPLv2+ as well in order to keep the resultant license of the hosted release files simple -- even if library users won't "link against" this command line tool. Therefore, I am asking permission from the current authors of this tool to loosen the license. At present, those people are: - John Snow (me!), 411/609 - Luiz Capitulino, Author, 97/609 - Daniel Berrangé, 81/609 - Eduardo Habkost, 10/609 - Marc-André Lureau, 6/609 - Fam Zheng, 3/609 - Cleber Rosa, 1/609 (All of which appear to have been written under redhat.com addresses.) Eduardo's fixes are largely automated from 2to3 conversion tools and may not necessarily constitute authorship, but his signature would put to rest any questions. Cleber's changes concern a single import statement change. Also won't hurt to ask. Signed-off-by: John Snow Reviewed-by: Marc-André Lureau Acked-by: Fam Zheng Acked-by: Luiz Capitulino Acked-by: Eduardo Habkost Acked-by: Daniel P. Berrangé Acked-by: Cleber Rosa Message-id: 20220325200438.2556381-4-js...@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/qmp_shell.py | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py index 35691494d0a..c23f1b19280 100644 --- a/python/qemu/aqmp/qmp_shell.py +++ b/python/qemu/aqmp/qmp_shell.py @@ -1,11 +1,12 @@ # -# Copyright (C) 2009, 2010 Red Hat Inc. +# Copyright (C) 2009-2022 Red Hat Inc. # # Authors: # Luiz Capitulino +# John Snow # -# This work is licensed under the terms of the GNU GPL, version 2. See -# the COPYING file in the top-level directory. +# This work is licensed under the terms of the GNU LGPL, version 2 or +# later. See the COPYING file in the top-level directory. # """ -- 2.34.1
[PULL 00/17] Python patches
The following changes since commit b1efff6bf031a93b5b8bf3912ddc720cc1653a61:
Merge tag 'pull-ppc-20220420-2' of https://gitlab.com/danielhb/qemu into
staging (2022-04-20 21:54:24 -0700)
are available in the Git repository at:
https://gitlab.com/jsnow/qemu.git tags/python-pull-request
for you to fetch changes up to 47430775ed1a48d7beb2c7b8d7feaab73104ec46:
python/qmp: remove pylint workaround from legacy.py (2022-04-21 11:01:00
-0400)
Python patches
This PR finalizes the switch from Luiz's QMP library to mine.
----
John Snow (17):
python/machine: permanently switch to AQMP
scripts/bench-block-job: switch to AQMP
iotests/mirror-top-perms: switch to AQMP
iotests: switch to AQMP
python/aqmp: add explicit GPLv2 license to legacy.py
python/aqmp: relicense as LGPLv2+
python/qmp-shell: relicense as LGPLv2+
python/aqmp-tui: relicense as LGPLv2+
python: temporarily silence pylint duplicate-code warnings
python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
python/aqmp: fully separate from qmp.QEMUMonitorProtocol
python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
python: remove the old QMP package
python: re-enable pylint duplicate-code warnings
python: rename qemu.aqmp to qemu.qmp
python: rename 'aqmp-tui' to 'qmp-tui'
python/qmp: remove pylint workaround from legacy.py
python/README.rst | 2 +-
python/qemu/qmp/README.rst| 9 -
python/qemu/aqmp/__init__.py | 59 ---
python/qemu/aqmp/legacy.py| 177 ---
python/qemu/aqmp/py.typed | 0
python/qemu/machine/machine.py| 18 +-
python/qemu/machine/qtest.py | 2 +-
python/qemu/qmp/__init__.py | 445 ++
python/qemu/{aqmp => qmp}/error.py| 0
python/qemu/{aqmp => qmp}/events.py | 2 +-
python/qemu/qmp/legacy.py | 315 +
python/qemu/{aqmp => qmp}/message.py | 0
python/qemu/{aqmp => qmp}/models.py | 0
python/qemu/{aqmp => qmp}/protocol.py | 4 +-
python/qemu/{aqmp => qmp}/qmp_client.py | 16 +-
python/qemu/{aqmp => qmp}/qmp_shell.py| 11 +-
.../qemu/{aqmp/aqmp_tui.py => qmp/qmp_tui.py} | 17 +-
python/qemu/{aqmp => qmp}/util.py | 0
python/qemu/utils/qemu_ga_client.py | 4 +-
python/qemu/utils/qom.py | 2 +-
python/qemu/utils/qom_common.py | 4 +-
python/qemu/utils/qom_fuse.py | 2 +-
python/setup.cfg | 11 +-
python/tests/protocol.py | 14 +-
scripts/cpu-x86-uarch-abi.py | 2 +-
scripts/device-crash-test | 4 +-
scripts/qmp/qmp-shell | 2 +-
scripts/qmp/qmp-shell-wrap| 2 +-
scripts/render_block_graph.py | 4 +-
scripts/simplebench/bench_block_job.py| 5 +-
tests/qemu-iotests/iotests.py | 3 +-
tests/qemu-iotests/tests/mirror-top-perms | 11 +-
32 files changed, 422 insertions(+), 725 deletions(-)
delete mode 100644 python/qemu/qmp/README.rst
delete mode 100644 python/qemu/aqmp/__init__.py
delete mode 100644 python/qemu/aqmp/legacy.py
delete mode 100644 python/qemu/aqmp/py.typed
rename python/qemu/{aqmp => qmp}/error.py (100%)
rename python/qemu/{aqmp => qmp}/events.py (99%)
create mode 100644 python/qemu/qmp/legacy.py
rename python/qemu/{aqmp => qmp}/message.py (100%)
rename python/qemu/{aqmp => qmp}/models.py (100%)
rename python/qemu/{aqmp => qmp}/protocol.py (99%)
rename python/qemu/{aqmp => qmp}/qmp_client.py (97%)
rename python/qemu/{aqmp => qmp}/qmp_shell.py (98%)
rename python/qemu/{aqmp/aqmp_tui.py => qmp/qmp_tui.py} (98%)
rename python/qemu/{aqmp => qmp}/util.py (100%)
--
2.34.1
Re: [PATCH 1/2] python/machine.py: upgrade vm.command() method
On Tue, Apr 19, 2022, 12:42 PM Daniel P. Berrangé wrote: > On Fri, Apr 08, 2022 at 08:02:13PM +0300, Vladimir Sementsov-Ogievskiy > wrote: > > The method is not popular, we prefer use vm.qmp() and then check > > success by hand.. But that's not optimal. To simplify movement to > > vm.command() support same interface improvements like in vm.qmp() and > > rename to shorter vm.cmd(). > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > --- > > python/qemu/machine/machine.py | 16 --- > > tests/qemu-iotests/256 | 34 > > tests/qemu-iotests/257 | 36 +- > > 3 files changed, 48 insertions(+), 38 deletions(-) > > > > diff --git a/python/qemu/machine/machine.py > b/python/qemu/machine/machine.py > > index 07ac5a710b..a3fb840b93 100644 > > --- a/python/qemu/machine/machine.py > > +++ b/python/qemu/machine/machine.py > > @@ -648,14 +648,24 @@ def qmp(self, cmd: str, > > self._quit_issued = True > > return ret > > > > -def command(self, cmd: str, > > -conv_keys: bool = True, > > -**args: Any) -> QMPReturnValue: > > +def cmd(self, cmd: str, > > FYI, the original 'command' name matches semantics of 'command' > in the QEMUMonitorProtocol class. The QEMUMonitorProtocol > class has a 'cmd' method that matches semantics of 'qmp' in > QEMUMachine. > > I think it is desirable to have consistency between naming in > the two classes. > Broadly agree. > The current use of both 'cmd' and 'command' in QEMUMonitorProtocol > is horrible naming though. How is anyone supposed to remember which > name raises the exception and which doesn't ? Urgh. > Also agree. > I agree with your desire to simplify things, but if anything I would > suggest we change both QEMUMonitorProtocol and QEMUMachine to have a > convention more like python's subprocess module: > > - 'cmd' runs without error checking, just returning the >reply data as is > > - 'check_cmd' calls 'cmd' but raises an exception on error. > Not sure I'm on board here, though. For what it's worth, in async qmp I added a single method "execute()", mimicking the name of the field used over the wire. It uses semantics like command() here, in that it raises an exception on error and returns only the response data and not the entire QMP response. I'm in favor of, broadly, an approach wherein the default behavior raises an exception and the caller must opt-in to squelch it; either via try-except or a check=False argument. It should be a conscious decision. (I realize this is not what the majority of iotests already does, but I consider that a problem to fix and not a feature. See also my recent efforts to make qemu_img() and qemu_io() raise a CalledProcessError by default to improve failed test diagnostics and remove logical errors from several iotests.) Over time, I want to migrate machine.py off of the legacy.py interface and onto the native async qmp. I believe using asyncio subprocess and asyncio qmp will give better failure characteristics and better error readouts. I've prototyped this a little, but it's a big project and there are still pre-requisites to hit before I worry about it too much. Maybe this summer I'll tackle it. Anyway, that's nobody else's problem right now, but I have a predisposition to not want to steer far away from what the async api provides. Just some roadmap info in case we need to collaborate better on the future of this module. --js >
[PATCH v3 10/12] iotests: remove qemu_io_pipe_and_status()
I know we just added it, sorry. This is done in favor of qemu_io() which
*also* returns the console output and status, but with more robust error
handling on failure.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 3 ---
1 file changed, 3 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index baf4995089a..e903c8ede00 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -365,9 +365,6 @@ def qemu_io(*args: str, check: bool = True, combine_stdio:
bool = True
return qemu_tool(*qemu_io_wrap_args(args),
check=check, combine_stdio=combine_stdio)
-def qemu_io_pipe_and_status(*args):
-return qemu_tool_pipe_and_status('qemu-io', qemu_io_wrap_args(args))
-
def qemu_io_log(*args: str) -> 'subprocess.CompletedProcess[str]':
result = qemu_io(*args, check=False)
log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
--
2.34.1
[PATCH v3 07/12] iotests: rebase qemu_io() on top of qemu_tool()
Rework qemu_io() to be analogous to qemu_img(); a function that requires
a return code of zero by default unless disabled explicitly.
Tests that use qemu_io():
030 040 041 044 055 056 093 124 129 132 136 148 149 151 152 163 165 205
209 219 236 245 248 254 255 257 260 264 280 298 300 302 304
image-fleecing migrate-bitmaps-postcopy-test migrate-bitmaps-test
migrate-during-backup migration-permissions
Test that use qemu_io_log():
242 245 255 274 303 307 nbd-reconnect-on-open
Copy-pastables for testing/verification:
./check -qcow2 030 040 041 044 055 056 124 129 132 151 152 163 165 209 \
219 236 242 245 248 254 255 257 260 264 274 \
280 298 300 302 303 304 307 image-fleecing \
migrate-bitmaps-postcopy-test migrate-bitmaps-test \
migrate-during-backup nbd-reconnect-on-open
./check -raw 093 136 148 migration-permissions
./check -nbd 205
# ./configure configure --disable-gnutls --enable-gcrypt
# this ALSO requires passwordless sudo.
./check -luks 149
# Just the tests that were edited in this commit:
./check -qcow2 030 040 242 245
./check -raw migration-permissions
./check -nbd 205
./check -luks 149
Signed-off-by: John Snow
---
tests/qemu-iotests/030| 85 +++
tests/qemu-iotests/149| 6 +-
tests/qemu-iotests/205| 4 +-
tests/qemu-iotests/245| 17 ++--
tests/qemu-iotests/iotests.py | 19 +++--
.../qemu-iotests/tests/migration-permissions | 4 +-
6 files changed, 81 insertions(+), 54 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 18eddcc7344..98595d47fec 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -64,16 +64,18 @@ class TestSingleDrive(iotests.QMPTestCase):
self.assert_no_active_block_jobs()
self.vm.shutdown()
-self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
- qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
- 'image file map does not match backing file after
streaming')
+self.assertEqual(
+qemu_io('-f', 'raw', '-c', 'map', backing_img).stdout,
+qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img).stdout,
+'image file map does not match backing file after streaming')
def test_stream_intermediate(self):
self.assert_no_active_block_jobs()
-self.assertNotEqual(qemu_io('-f', 'raw', '-rU', '-c', 'map',
backing_img),
-qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map',
mid_img),
-'image file map matches backing file before
streaming')
+self.assertNotEqual(
+qemu_io('-f', 'raw', '-rU', '-c', 'map', backing_img).stdout,
+qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', mid_img).stdout,
+'image file map matches backing file before streaming')
result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
self.assert_qmp(result, 'return', {})
@@ -83,9 +85,10 @@ class TestSingleDrive(iotests.QMPTestCase):
self.assert_no_active_block_jobs()
self.vm.shutdown()
-self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
- qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img),
- 'image file map does not match backing file after
streaming')
+self.assertEqual(
+qemu_io('-f', 'raw', '-c', 'map', backing_img).stdout,
+qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img).stdout,
+'image file map does not match backing file after streaming')
def test_stream_pause(self):
self.assert_no_active_block_jobs()
@@ -113,15 +116,17 @@ class TestSingleDrive(iotests.QMPTestCase):
self.assert_no_active_block_jobs()
self.vm.shutdown()
-self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
- qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
- 'image file map does not match backing file after
streaming')
+self.assertEqual(
+qemu_io('-f', 'raw', '-c', 'map', backing_img).stdout,
+
[PATCH v3 08/12] iotests/migration-permissions: use assertRaises() for qemu_io() negative test
Modify this test to use assertRaises for its negative testing of
qemu_io. If the exception raised does not match the one we tell it to
expect, we get *that* exception unhandled. If we get no exception, we
get a unittest assertion failure and the provided emsg printed to
screen.
If we get the CalledProcessError exception but the output is not what we
expect, we re-raise the original CalledProcessError.
Tidy.
(Note: Yes, you can reference "with" objects after that block ends; it
just means that ctx.__exit__(...) will have been called on it. It does
not *actually* go out of scope. unittests expects you to want to inspect
the Exception object, so they leave it defined post-exit.)
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Tested-by: Eric Blake
Reviewed-by: Hanna Reitz
---
.../qemu-iotests/tests/migration-permissions | 28 +--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/tests/qemu-iotests/tests/migration-permissions
b/tests/qemu-iotests/tests/migration-permissions
index c7afb1bd2c1..4e1da369c94 100755
--- a/tests/qemu-iotests/tests/migration-permissions
+++ b/tests/qemu-iotests/tests/migration-permissions
@@ -18,6 +18,8 @@
#
import os
+from subprocess import CalledProcessError
+
import iotests
from iotests import imgfmt, qemu_img_create, qemu_io
@@ -69,13 +71,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
def test_post_migration_permissions(self):
# Try to access the image R/W, which should fail because virtio-blk
# has not been configured with share-rw=on
-log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
-if not log.strip():
-print('ERROR (pre-migration): qemu-io should not be able to '
- 'access this image, but it reported no error')
-else:
-# This is the expected output
-assert 'Is another process using the image' in log
+emsg = ('ERROR (pre-migration): qemu-io should not be able to '
+'access this image, but it reported no error')
+with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
+qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+if 'Is another process using the image' not in ctx.exception.stdout:
+raise ctx.exception
# Now migrate the VM
self.vm_s.qmp('migrate', uri=f'unix:{mig_sock}')
@@ -84,13 +85,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
# Try the same qemu-io access again, verifying that the WRITE
# permission remains unshared
-log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
-if not log.strip():
-print('ERROR (post-migration): qemu-io should not be able to '
- 'access this image, but it reported no error')
-else:
-# This is the expected output
-assert 'Is another process using the image' in log
+emsg = ('ERROR (post-migration): qemu-io should not be able to '
+'access this image, but it reported no error')
+with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
+qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+if 'Is another process using the image' not in ctx.exception.stdout:
+raise ctx.exception
if __name__ == '__main__':
--
2.34.1
[PATCH v3 06/12] iotests: create generic qemu_tool() function
reimplement qemu_img() in terms of qemu_tool() in preparation for doing the same with qemu_io(). Signed-off-by: John Snow Reviewed-by: Eric Blake Reviewed-by: Hanna Reitz --- tests/qemu-iotests/iotests.py | 32 +--- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index fcec3e51e51..e4e18a5889d 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -207,15 +207,13 @@ def qemu_img_create_prepare_args(args: List[str]) -> List[str]: return result -def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True - ) -> 'subprocess.CompletedProcess[str]': + +def qemu_tool(*args: str, check: bool = True, combine_stdio: bool = True + ) -> 'subprocess.CompletedProcess[str]': """ -Run qemu_img and return the status code and console output. +Run a qemu tool and return its status code and console output. -This function always prepends QEMU_IMG_OPTIONS and may further alter -the args for 'create' commands. - -:param args: command-line arguments to qemu-img. +:param args: full command line to run. :param check: Enforce a return code of zero. :param combine_stdio: set to False to keep stdout/stderr separated. @@ -232,10 +230,8 @@ def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True properties. If streams are not combined, it will also have a stderr property. """ -full_args = qemu_img_args + qemu_img_create_prepare_args(list(args)) - subp = subprocess.run( -full_args, +args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT if combine_stdio else subprocess.PIPE, universal_newlines=True, @@ -244,7 +240,7 @@ def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True if check and subp.returncode or (subp.returncode < 0): raise VerboseProcessError( -subp.returncode, full_args, +subp.returncode, args, output=subp.stdout, stderr=subp.stderr, ) @@ -252,6 +248,20 @@ def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True return subp +def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True + ) -> 'subprocess.CompletedProcess[str]': +""" +Run QEMU_IMG_PROG and return its status code and console output. + +This function always prepends QEMU_IMG_OPTIONS and may further alter +the args for 'create' commands. + +See `qemu_tool()` for greater detail. +""" +full_args = qemu_img_args + qemu_img_create_prepare_args(list(args)) +return qemu_tool(*full_args, check=check, combine_stdio=combine_stdio) + + def ordered_qmp(qmsg, conv_keys=True): # Dictionaries are not ordered prior to 3.6, therefore: if isinstance(qmsg, list): -- 2.34.1
[PATCH v3 09/12] iotests/image-fleecing: switch to qemu_io()
This test expects failure ... but only sometimes. When? Why?
It's for reads of a region not defined by a bitmap. Adjust the test to
be more explicit about what it expects to fail and why.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/tests/image-fleecing | 28 +
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/tests/image-fleecing
b/tests/qemu-iotests/tests/image-fleecing
index b7e50761044..ac749702f8e 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -22,9 +22,10 @@
#
# Creator/Owner: John Snow
+from subprocess import CalledProcessError
+
import iotests
-from iotests import log, qemu_img, qemu_io, qemu_io_silent, \
-qemu_io_pipe_and_status
+from iotests import log, qemu_img, qemu_io, qemu_io_silent
iotests.script_initialize(
supported_fmts=['qcow2'],
@@ -185,10 +186,14 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
for p in patterns + zeroes:
cmd = 'read -P%s %s %s' % p
log(cmd)
-out, ret = qemu_io_pipe_and_status('-r', '-f', 'raw', '-c', cmd,
- nbd_uri)
-if ret != 0:
-print(out)
+
+try:
+qemu_io('-r', '-f', 'raw', '-c', cmd, nbd_uri)
+except CalledProcessError as exc:
+if bitmap and p in zeroes:
+log(exc.stdout)
+else:
+raise
log('')
log('--- Testing COW ---')
@@ -228,9 +233,14 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
args += [target_img_path]
else:
args += ['-f', 'raw', nbd_uri]
-out, ret = qemu_io_pipe_and_status(*args)
-if ret != 0:
-print(out)
+
+try:
+qemu_io(*args)
+except CalledProcessError as exc:
+if bitmap and p in zeroes:
+log(exc.stdout)
+else:
+raise
log('')
log('--- Cleanup ---')
--
2.34.1
[PATCH v3 04/12] iotests/040: Don't check image pattern on zero-length image
qemu-io fails on read/write beyond end-of-file on raw images, so skip
these invocations when running the zero-length image tests.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/040 | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index adf5815781e..c4a90937dcb 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -86,8 +86,10 @@ class TestSingleDrive(ImageCommitTestCase):
qemu_img('create', '-f', iotests.imgfmt,
'-o', 'backing_file=%s' % mid_img,
'-F', iotests.imgfmt, test_img)
-qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
-qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288',
mid_img)
+if self.image_len:
+qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
+qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288',
+mid_img)
self.vm = iotests.VM().add_drive(test_img,
"node-name=top,backing.node-name=mid,backing.backing.node-name=base",
interface="none")
self.vm.add_device('virtio-scsi')
self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
@@ -101,11 +103,15 @@ class TestSingleDrive(ImageCommitTestCase):
def test_commit(self):
self.run_commit_test(mid_img, backing_img)
+if not self.image_len:
+return
qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
def test_commit_node(self):
self.run_commit_test("mid", "base", node_names=True)
+if not self.image_len:
+return
qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
@@ -192,11 +198,15 @@ class TestSingleDrive(ImageCommitTestCase):
def test_top_is_active(self):
self.run_commit_test(test_img, backing_img, need_ready=True)
+if not self.image_len:
+return
qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
def test_top_is_default_active(self):
self.run_default_commit_test()
+if not self.image_len:
+return
qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
--
2.34.1
[PATCH v3 12/12] iotests: make qemu_io_log() check return codes by default
Just like qemu_img_log(), upgrade qemu_io_log() to enforce a return code
of zero by default.
Tests that use qemu_io_log(): 242 245 255 274 303 307 nbd-reconnect-on-open
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 5 +++--
tests/qemu-iotests/tests/nbd-reconnect-on-open | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1d103a38722..1a3662db0b1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -365,8 +365,9 @@ def qemu_io(*args: str, check: bool = True, combine_stdio:
bool = True
return qemu_tool(*qemu_io_wrap_args(args),
check=check, combine_stdio=combine_stdio)
-def qemu_io_log(*args: str) -> 'subprocess.CompletedProcess[str]':
-result = qemu_io(*args, check=False)
+def qemu_io_log(*args: str, check: bool = True
+) -> 'subprocess.CompletedProcess[str]':
+result = qemu_io(*args, check=check)
log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
return result
diff --git a/tests/qemu-iotests/tests/nbd-reconnect-on-open
b/tests/qemu-iotests/tests/nbd-reconnect-on-open
index 8be721a24fb..d0b401b0602 100755
--- a/tests/qemu-iotests/tests/nbd-reconnect-on-open
+++ b/tests/qemu-iotests/tests/nbd-reconnect-on-open
@@ -39,7 +39,7 @@ def check_fail_to_connect(open_timeout):
log(f'Check fail to connect with {open_timeout} seconds of timeout')
start_t = time.time()
-qemu_io_log(*create_args(open_timeout))
+qemu_io_log(*create_args(open_timeout), check=False)
delta_t = time.time() - start_t
max_delta = open_timeout + 0.2
--
2.34.1
[PATCH v3 03/12] iotests: Don't check qemu_io() output for specific error strings
A forthcoming commit updates qemu_io() to raise an exception on non-zero
return by default, and changes its return type.
In preparation, simplify some calls to qemu_io() that assert that
specific error message strings do not appear in qemu-io's
output. Asserting that all of these calls return a status code of zero
will be a more robust way to guard against failure.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/040 | 33 -
tests/qemu-iotests/056 | 2 +-
2 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 0e1cfd7e49d..adf5815781e 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -101,13 +101,13 @@ class TestSingleDrive(ImageCommitTestCase):
def test_commit(self):
self.run_commit_test(mid_img, backing_img)
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0
524288', backing_img).find("verification failed"))
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288
524288', backing_img).find("verification failed"))
+qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
def test_commit_node(self):
self.run_commit_test("mid", "base", node_names=True)
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0
524288', backing_img).find("verification failed"))
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288
524288', backing_img).find("verification failed"))
+qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
@iotests.skip_if_unsupported(['throttle'])
def test_commit_with_filter_and_quit(self):
@@ -192,13 +192,13 @@ class TestSingleDrive(ImageCommitTestCase):
def test_top_is_active(self):
self.run_commit_test(test_img, backing_img, need_ready=True)
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0
524288', backing_img).find("verification failed"))
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288
524288', backing_img).find("verification failed"))
+qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
def test_top_is_default_active(self):
self.run_default_commit_test()
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0
524288', backing_img).find("verification failed"))
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288
524288', backing_img).find("verification failed"))
+qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
def test_top_and_base_reversed(self):
self.assert_no_active_block_jobs()
@@ -334,8 +334,8 @@ class TestRelativePaths(ImageCommitTestCase):
def test_commit(self):
self.run_commit_test(self.mid_img, self.backing_img)
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0
524288', self.backing_img_abs).find("verification failed"))
-self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288
524288', self.backing_img_abs).find("verification failed"))
+qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288',
self.backing_img_abs)
+qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288',
self.backing_img_abs)
def test_device_not_found(self):
result = self.vm.qmp('block-commit', device='nonexistent', top='%s' %
self.mid_img)
@@ -361,8 +361,8 @@ class TestRelativePaths(ImageCommitTestCase):
def test_top_is_active(self):
self.run_commit_test(self.test_img, self.backing_img)
-self.assertEqual(-1, q
[PATCH v3 01/12] iotests: replace calls to log(qemu_io(...)) with qemu_io_log()
This makes these callsites a little simpler, but the real motivation is
a forthcoming commit will change the return type of qemu_io(), so removing
users of the return value now is helpful.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/242 | 6 +++---
tests/qemu-iotests/255 | 4 +---
tests/qemu-iotests/303 | 4 ++--
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index b3afd36d724..c89f0c6cb32 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -22,8 +22,8 @@
import iotests
import json
import struct
-from iotests import qemu_img_create, qemu_io, qemu_img_info, \
-file_path, img_info_log, log, filter_qemu_io
+from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
+file_path, img_info_log, log
iotests.script_initialize(supported_fmts=['qcow2'],
supported_protocols=['file'],
@@ -61,7 +61,7 @@ def add_bitmap(bitmap_number, persistent, disabled):
def write_to_disk(offset, size):
write = 'write {} {}'.format(offset, size)
-log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+qemu_io_log('-c', write, disk)
def toggle_flag(offset):
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index f86fa851b62..88b29d64b44 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -95,9 +95,7 @@ with iotests.FilePath('src.qcow2') as src_path, \
iotests.qemu_img_create('-f', iotests.imgfmt, src_path, size_str)
iotests.qemu_img_create('-f', iotests.imgfmt, dst_path, size_str)
-iotests.log(iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write 0 1M',
-src_path),
-filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
+iotests.qemu_io_log('-f', iotests.imgfmt, '-c', 'write 0 1M', src_path),
vm.add_object('throttle-group,x-bps-read=4096,id=throttle0')
diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 93aa5ce9b7d..32128b1d322 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -21,7 +21,7 @@
import iotests
import subprocess
-from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+from iotests import qemu_img_create, qemu_io_log, file_path, log
iotests.script_initialize(supported_fmts=['qcow2'],
unsupported_imgopts=['refcount_bits', 'compat'])
@@ -43,7 +43,7 @@ def create_bitmap(bitmap_number, disabled):
def write_to_disk(offset, size):
write = f'write {offset} {size}'
-log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+qemu_io_log('-c', write, disk)
def add_bitmap(num, begin, end, disabled):
--
2.34.1
[PATCH v3 02/12] iotests/163: Fix broken qemu-io invocation
The 'read' commands to qemu-io were malformed, and this invocation only
worked by coincidence because the error messages were identical. Oops.
There's no point in checking the patterning of the reference image, so
just check the empty image by itself instead.
(Note: as of this commit, nothing actually enforces that this command
completes successfully, but a forthcoming commit in this series will
enforce that qemu_io() must have a zero status code.)
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/163 | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
index e4cd4b230f3..c94ad16f4a7 100755
--- a/tests/qemu-iotests/163
+++ b/tests/qemu-iotests/163
@@ -113,10 +113,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
qemu_img('resize', '-f', iotests.imgfmt, '--shrink', test_img,
self.shrink_size)
-self.assertEqual(
-qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, test_img),
-qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, check_img),
-"Verifying image content")
+qemu_io('-c', f"read -P 0x00 0 {self.shrink_size}", test_img)
self.image_verify()
--
2.34.1
[PATCH v3 11/12] iotests: remove qemu_io_silent() and qemu_io_silent_check().
Like qemu-img, qemu-io returning 0 should be the norm and not the
exception. Remove all calls to qemu_io_silent that just assert the
return code is zero (That's every last call, as it turns out), and
replace them with a normal qemu_io() call.
qemu_io_silent_check() appeared to have been unused already.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
---
tests/qemu-iotests/216| 12 +-
tests/qemu-iotests/218| 5 ++---
tests/qemu-iotests/224| 4 ++--
tests/qemu-iotests/258| 11 +-
tests/qemu-iotests/298| 15 +
tests/qemu-iotests/310| 22 +--
tests/qemu-iotests/iotests.py | 16 --
tests/qemu-iotests/tests/image-fleecing | 4 ++--
.../tests/mirror-ready-cancel-error | 2 +-
.../qemu-iotests/tests/stream-error-on-reset | 4 ++--
10 files changed, 37 insertions(+), 58 deletions(-)
diff --git a/tests/qemu-iotests/216 b/tests/qemu-iotests/216
index c531abfded9..311e02af3a7 100755
--- a/tests/qemu-iotests/216
+++ b/tests/qemu-iotests/216
@@ -21,7 +21,7 @@
# Creator/Owner: Hanna Reitz
import iotests
-from iotests import log, qemu_img, qemu_io_silent
+from iotests import log, qemu_img, qemu_io
# Need backing file support
iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'],
@@ -52,10 +52,10 @@ with iotests.FilePath('base.img') as base_img_path, \
log('')
qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
-assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
+qemu_io(base_img_path, '-c', 'write -P 1 0M 1M')
qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
'-F', iotests.imgfmt, top_img_path)
-assert qemu_io_silent(top_img_path, '-c', 'write -P 2 1M 1M') == 0
+qemu_io(top_img_path, '-c', 'write -P 2 1M 1M')
log('Done')
@@ -110,8 +110,8 @@ with iotests.FilePath('base.img') as base_img_path, \
log('--- Checking COR result ---')
log('')
-assert qemu_io_silent(base_img_path, '-c', 'discard 0 64M') == 0
-assert qemu_io_silent(top_img_path, '-c', 'read -P 1 0M 1M') == 0
-assert qemu_io_silent(top_img_path, '-c', 'read -P 2 1M 1M') == 0
+qemu_io(base_img_path, '-c', 'discard 0 64M')
+qemu_io(top_img_path, '-c', 'read -P 1 0M 1M')
+qemu_io(top_img_path, '-c', 'read -P 2 1M 1M')
log('Done')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 8345793902e..6320c4cb56e 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -28,7 +28,7 @@
# Creator/Owner: Hanna Reitz
import iotests
-from iotests import log, qemu_img, qemu_io_silent
+from iotests import log, qemu_img, qemu_io
iotests.script_initialize(supported_fmts=['qcow2', 'raw'])
@@ -146,8 +146,7 @@ with iotests.VM() as vm, \
iotests.FilePath('src.img') as src_img_path:
qemu_img('create', '-f', iotests.imgfmt, src_img_path, '64M')
-assert qemu_io_silent('-f', iotests.imgfmt, src_img_path,
- '-c', 'write -P 42 0M 64M') == 0
+qemu_io('-f', iotests.imgfmt, src_img_path, '-c', 'write -P 42 0M 64M')
vm.launch()
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
index 4df5157e8df..542d0eefa60 100755
--- a/tests/qemu-iotests/224
+++ b/tests/qemu-iotests/224
@@ -22,7 +22,7 @@
# Creator/Owner: Hanna Reitz
import iotests
-from iotests import log, qemu_img, qemu_io_silent, filter_qmp_testfiles, \
+from iotests import log, qemu_img, qemu_io, filter_qmp_testfiles, \
filter_qmp_imgfmt
import json
@@ -54,7 +54,7 @@ for filter_node_name in False, True:
'-F', iotests.imgfmt, top_img_path)
# Something to commit
-assert qemu_io_silent(mid_img_path, '-c', 'write -P 1 0 1M') == 0
+qemu_io(mid_img_path, '-c', 'write -P 1 0 1M')
vm.launch()
diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index cfd536d6dce..73d4af645f0 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -21,7 +21,7 @@
# Creator/Owner: Hanna Reitz
import iotests
-from iotests import log, qemu_img, qemu_io_silent, \
+from iotests import log, qemu_img, qemu_io, \
filter_qmp_testfiles, filter_qmp_imgfmt
# Returns a node for bl
[PATCH v3 05/12] iotests/040: Fix TestCommitWithFilters test
Without this change, asserting that qemu_io always returns 0 causes this
test to fail in a way we happened not to be catching previously:
qemu.utils.VerboseProcessError: Command
'('/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/../../qemu-io',
'--cache', 'writeback', '--aio', 'threads', '-f', 'qcow2', '-c',
'read -P 4 3M 1M',
'/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img')'
returned non-zero exit status 1.
┏━ output
┃ qemu-io: can't open device
┃ /home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img:
┃ Could not open backing file: Could not open backing file: Throttle
┃ group 'tg' does not exist
┗━
The commit jobs changes the backing file string stored in the image file
header belonging to the node above the commit’s top node to point to the
commit target (the base node). QEMU tries to be as accurate as
possible, and so in these test cases will include the filter that is
part of the block graph in that backing file string (by virtue of making
it a json:{} description of the post-commit subgraph). This makes
little sense outside of QEMU, though: Specifically, the throttle node in
that subgraph will dearly miss its supposedly associated throttle group
object.
When starting the commit job, we can specify a custom backing file
string to write into said image file, so let’s use that feature to write
the plain filename of the backing chain’s next actual image file there.
Explicitly provide the backing file so that opening the file outside of
QEMU (Where we will not have throttle groups) will succeed.
Signed-off-by: Hanna Reitz
Signed-off-by: John Snow
Reviewed-by: Eric Blake
---
tests/qemu-iotests/040 | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index c4a90937dcb..30eb97829ea 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -836,7 +836,8 @@ class TestCommitWithFilters(iotests.QMPTestCase):
job_id='commit',
device='top-filter',
top_node='cow-2',
- base_node='cow-1')
+ base_node='cow-1',
+ backing_file=self.img1)
self.assert_qmp(result, 'return', {})
self.wait_until_completed(drive='commit')
@@ -852,7 +853,8 @@ class TestCommitWithFilters(iotests.QMPTestCase):
job_id='commit',
device='top-filter',
top_node='cow-1',
- base_node='cow-0')
+ base_node='cow-0',
+ backing_file=self.img0)
self.assert_qmp(result, 'return', {})
self.wait_until_completed(drive='commit')
--
2.34.1
[PATCH v3 00/12] iotests: add enhanced debugging info to qemu-io failures
GitLab: https://gitlab.com/jsnow/qemu/-/commits/iotests_qemu_io_diagnostics Howdy, This series does for qemu_io() what we've done for qemu_img() and makes it a function that checks the return code by default and raises an Exception when things do not go according to plan. This series removes qemu_io_pipe_and_status(), qemu_io_silent(), and qemu_io_silent_check() in favor of just qemu_io(). V3: - Rebased - Squashed the patches that I said I would Note: check-tox job will fail right now, it's unrelated to this series; see https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg07149.html V2: - Fixed 040 - Fixed 245 on tmpfs - Fixed tests/image-fleecing - I expect to respin a v3 to: (A) Fix the commit message on the 040 fix (B) Squash patches 7-12. Notes: - There are a few remaining uses of qemu-io that don't go through qemu_io; QemuIoInteractive is a user that is used in 205, 298, 299, and 307. It ... did not appear worth it to morph qemu_tool_popen into something that could be used by both QemuIoInteractive *and* qemu_io(), so I left it alone. It's probably fine for now. (But it does bother me, a little.) - qemu_io_popen itself is used by the nbd-reconnect-on-open test, and it seems like a legitimate use -- it wants concurrency. Like the above problem, I couldn't find a way to bring it into the fold, so I didn't. (Meh.) I eventually plan to add asyncio subprocess management to machine.py, and I could tackle stuff like this then. It's not worth it now. (Maybe I'll bring these in under the fold the next time I get bored, but I think it's not worth the trouble right now, there are very few users. I did try, but the benefit to VerboseProcessError is that it includes stdout/stderr. When using Popen with pipes you lose access to that information in the management context. Popen does not natively buffer stdout/stderr, so we'd have to fall back to just using a regular CalledProcessError. I think I'd have to extend Popen and add buffering. I think that's something for later.) (I tried doing the above and it's definitely more hassle than it's worth right now.) John Snow (12): iotests: replace calls to log(qemu_io(...)) with qemu_io_log() iotests/163: Fix broken qemu-io invocation iotests: Don't check qemu_io() output for specific error strings iotests/040: Don't check image pattern on zero-length image iotests/040: Fix TestCommitWithFilters test iotests: create generic qemu_tool() function iotests: rebase qemu_io() on top of qemu_tool() iotests/migration-permissions: use assertRaises() for qemu_io() negative test iotests/image-fleecing: switch to qemu_io() iotests: remove qemu_io_pipe_and_status() iotests: remove qemu_io_silent() and qemu_io_silent_check(). iotests: make qemu_io_log() check return codes by default tests/qemu-iotests/030| 85 +++ tests/qemu-iotests/040| 53 +++- tests/qemu-iotests/056| 2 +- tests/qemu-iotests/149| 6 +- tests/qemu-iotests/163| 5 +- tests/qemu-iotests/205| 4 +- tests/qemu-iotests/216| 12 +-- tests/qemu-iotests/218| 5 +- tests/qemu-iotests/224| 4 +- tests/qemu-iotests/242| 6 +- tests/qemu-iotests/245| 17 ++-- tests/qemu-iotests/255| 4 +- tests/qemu-iotests/258| 11 ++- tests/qemu-iotests/298| 15 ++-- tests/qemu-iotests/303| 4 +- tests/qemu-iotests/310| 22 ++--- tests/qemu-iotests/iotests.py | 69 --- tests/qemu-iotests/tests/image-fleecing | 30 --- .../qemu-iotests/tests/migration-permissions | 28 +++--- .../tests/mirror-ready-cancel-error | 2 +- .../qemu-iotests/tests/nbd-reconnect-on-open | 2 +- .../qemu-iotests/tests/stream-error-on-reset | 4 +- 22 files changed, 210 insertions(+), 180 deletions(-) -- 2.34.1
Re: [PATCH v2 05/17] iotests/040: Fix TestCommitWithFilters test
On Thu, Mar 24, 2022 at 9:33 PM Eric Blake wrote:
>
> On Thu, Mar 24, 2022 at 02:30:06PM -0400, John Snow wrote:
> > Without this change, asserting that qemu_io always returns 0 causes this
> > test to fail in a way we happened not to be catching previously:
> >
> > qemu.utils.VerboseProcessError: Command
> > '('/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/../../qemu-io',
> > '--cache', 'writeback', '--aio', 'threads', '-f', 'qcow2', '-c',
> > 'read -P 4 3M 1M',
> > '/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img')'
> > returned non-zero exit status 1.
> > ┏━ output
> > ┃ qemu-io: can't open device
> > ┃ /home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img:
> > ┃ Could not open backing file: Could not open backing file: Throttle
> > ┃ group 'tg' does not exist
> > ┗━
> >
> > Explicitly provide the backing file so that opening the file outside of
> > QEMU (Where we will not have throttle groups) will succeed.
> >
> > [Patch entirely written by Hanna but I don't have her S-o-B]
>
> Yeah, you'll want that.
>
> > [My commit message is probably also garbage, sorry]
>
> No, it was actually decent.
>
> > [Feel free to suggest a better one]
> > [I hope your day is going well]
> > Signed-off-by: John Snow
> >
> > Signed-off-by: John Snow
>
> So giving your S-o-b twice makes up for it, right ;)
This happens when I add a '---' myself into the commit message, and
git-publish sees that the end of the commit message doesn't have a
S-o-B and adds one into the ignored region.
Haven't bothered to fix it yet.
>
> Well, you did say v3 would fix this. But while you're having fun
> fixing it, you can add:
>
> Reviewed-by: Eric Blake
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
Re: [PATCH v2 0/9] Python: Remove synchronous QMP library
On Wed, Mar 30, 2022 at 1:28 PM John Snow wrote:
>
> Based-on: https://gitlab.com/jsnow/qemu/-/tree/python/
> GitLab: https://gitlab.com/jsnow/qemu/-/tree/python-qmp-legacy-switch-pt1c
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/505169095
>
> Hi, this series is part of an effort to publish the qemu.qmp package on
> PyPI. It is the first of three series to complete this work:
>
> --> (1) Switch the new async QMP library in as python/qemu/qmp
> (2) Fork python/qemu/qmp out into its own repository,
> with updated GitLab CI/CD targets to build packages.
> (3) Update qemu.git to install qemu.qmp from PyPI,
> and then delete python/qemu/qmp.
>
> This series finalizes swapping out the old QMP library for the new
> one. This leaves us with just one QMP library to worry about. It also
> implements the rename of "qemu.aqmp" to "qemu.qmp".
>
> This is the last patch series before I perform the actual fork.
>
> These patches are (mostly) reviewed, so I'll likely stage these fairly
> quickly barring any objections. The plan is to submit them as soon as
> the tree re-opens to help prevent rot while I work on the fork.
>
> John Snow (9):
> python: temporarily silence pylint duplicate-code warnings
> python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
> python/aqmp: fully separate from qmp.QEMUMonitorProtocol
> python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
> python: remove the old QMP package
> python: re-enable pylint duplicate-code warnings
> python: rename qemu.aqmp to qemu.qmp
> python: rename 'aqmp-tui' to 'qmp-tui'
> python/qmp: remove pylint workaround from legacy.py
>
> python/README.rst | 2 +-
> python/qemu/qmp/README.rst| 9 -
> python/qemu/aqmp/__init__.py | 59 ---
> python/qemu/aqmp/legacy.py| 188
> python/qemu/aqmp/py.typed | 0
> python/qemu/machine/machine.py| 4 +-
> python/qemu/machine/qtest.py | 2 +-
> python/qemu/qmp/__init__.py | 445 ++
> python/qemu/{aqmp => qmp}/error.py| 0
> python/qemu/{aqmp => qmp}/events.py | 2 +-
> python/qemu/qmp/legacy.py | 315 +
> python/qemu/{aqmp => qmp}/message.py | 0
> python/qemu/{aqmp => qmp}/models.py | 0
> python/qemu/{aqmp => qmp}/protocol.py | 4 +-
> python/qemu/{aqmp => qmp}/qmp_client.py | 16 +-
> python/qemu/{aqmp => qmp}/qmp_shell.py| 4 +-
> .../qemu/{aqmp/aqmp_tui.py => qmp/qmp_tui.py} | 15 +-
> python/qemu/{aqmp => qmp}/util.py | 0
> python/qemu/utils/qemu_ga_client.py | 4 +-
> python/qemu/utils/qom.py | 2 +-
> python/qemu/utils/qom_common.py | 4 +-
> python/qemu/utils/qom_fuse.py | 2 +-
> python/setup.cfg | 11 +-
> python/tests/protocol.py | 14 +-
> scripts/cpu-x86-uarch-abi.py | 2 +-
> scripts/device-crash-test | 4 +-
> scripts/qmp/qmp-shell | 2 +-
> scripts/qmp/qmp-shell-wrap| 2 +-
> scripts/render_block_graph.py | 4 +-
> scripts/simplebench/bench_block_job.py| 2 +-
> tests/qemu-iotests/iotests.py | 2 +-
> tests/qemu-iotests/tests/mirror-top-perms | 4 +-
> 32 files changed, 409 insertions(+), 715 deletions(-)
> delete mode 100644 python/qemu/qmp/README.rst
> delete mode 100644 python/qemu/aqmp/__init__.py
> delete mode 100644 python/qemu/aqmp/legacy.py
> delete mode 100644 python/qemu/aqmp/py.typed
> rename python/qemu/{aqmp => qmp}/error.py (100%)
> rename python/qemu/{aqmp => qmp}/events.py (99%)
> create mode 100644 python/qemu/qmp/legacy.py
> rename python/qemu/{aqmp => qmp}/message.py (100%)
> rename python/qemu/{aqmp => qmp}/models.py (100%)
> rename python/qemu/{aqmp => qmp}/protocol.py (99%)
> rename python/qemu/{aqmp => qmp}/qmp_client.py (97%)
> rename python/qemu/{aqmp => qmp}/qmp_shell.py (99%)
> rename python/qemu/{aqmp/aqmp_tui.py => qmp/qmp_tui.py} (98%)
> rename python/qemu/{aqmp => qmp}/util.py (100%)
>
> --
> 2.34.1
>
Thanks, I've tentatively queued this on my Python branch. It won't be
going out until the tree opens again, so there's some time yet to
lodge a formal complaint. O:-)
--js
[PATCH v2 8/9] python: rename 'aqmp-tui' to 'qmp-tui'
This is the last vestige of the "aqmp" moniker surviving in the tree; remove it.
Signed-off-by: John Snow
Reviewed-by: Beraldo Leal
---
python/qemu/qmp/{aqmp_tui.py => qmp_tui.py} | 12 ++--
python/setup.cfg| 6 +++---
2 files changed, 9 insertions(+), 9 deletions(-)
rename python/qemu/qmp/{aqmp_tui.py => qmp_tui.py} (98%)
diff --git a/python/qemu/qmp/aqmp_tui.py b/python/qemu/qmp/qmp_tui.py
similarity index 98%
rename from python/qemu/qmp/aqmp_tui.py
rename to python/qemu/qmp/qmp_tui.py
index 59d3036be38..ce239d8979b 100644
--- a/python/qemu/qmp/aqmp_tui.py
+++ b/python/qemu/qmp/qmp_tui.py
@@ -6,13 +6,13 @@
# This work is licensed under the terms of the GNU LGPL, version 2 or
# later. See the COPYING file in the top-level directory.
"""
-AQMP TUI
+QMP TUI
-AQMP TUI is an asynchronous interface built on top the of the AQMP library.
+QMP TUI is an asynchronous interface built on top the of the QMP library.
It is the successor of QMP-shell and is bought-in as a replacement for it.
-Example Usage: aqmp-tui
-Full Usage: aqmp-tui --help
+Example Usage: qmp-tui
+Full Usage: qmp-tui --help
"""
import argparse
@@ -129,7 +129,7 @@ def has_handler_type(logger: logging.Logger,
class App(QMPClient):
"""
-Implements the AQMP TUI.
+Implements the QMP TUI.
Initializes the widgets and starts the urwid event loop.
@@ -612,7 +612,7 @@ def main() -> None:
Driver of the whole script, parses arguments, initialize the TUI and
the logger.
"""
-parser = argparse.ArgumentParser(description='AQMP TUI')
+parser = argparse.ArgumentParser(description='QMP TUI')
parser.add_argument('qmp_server', help='Address of the QMP server. '
'Format ')
parser.add_argument('--num-retries', type=int, default=10,
diff --git a/python/setup.cfg b/python/setup.cfg
index 773e51b34e7..e877ea56475 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -51,7 +51,7 @@ devel =
fuse =
fusepy >= 2.0.4
-# AQMP TUI dependencies
+# QMP TUI dependencies
tui =
urwid >= 2.1.2
urwid-readline >= 0.13
@@ -68,7 +68,7 @@ console_scripts =
qemu-ga-client = qemu.utils.qemu_ga_client:main
qmp-shell = qemu.qmp.qmp_shell:main
qmp-shell-wrap = qemu.qmp.qmp_shell:main_wrap
-aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
+qmp-tui = qemu.qmp.qmp_tui:main [tui]
[flake8]
extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's
@@ -84,7 +84,7 @@ namespace_packages = True
# fusepy has no type stubs:
allow_subclassing_any = True
-[mypy-qemu.qmp.aqmp_tui]
+[mypy-qemu.qmp.qmp_tui]
# urwid and urwid_readline have no type stubs:
allow_subclassing_any = True
--
2.34.1
[PATCH v2 9/9] python/qmp: remove pylint workaround from legacy.py
Pylint upgraded recently (2.13.z) and having a pylint: disable comment in the middle of an argument field causes it some grief (It appears to stop parsing when it encounters it, causing some syntax problems). Since the duplicate line threshold was bumped up in 22305c2a081b, we don't need this workaround anymore. Drop it. Signed-off-by: John Snow --- python/qemu/qmp/legacy.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py index a8629b44dff..03b5574618f 100644 --- a/python/qemu/qmp/legacy.py +++ b/python/qemu/qmp/legacy.py @@ -106,8 +106,6 @@ def __enter__(self: _T) -> _T: return self def __exit__(self, - # pylint: disable=duplicate-code - # see https://github.com/PyCQA/pylint/issues/3619 exc_type: Optional[Type[BaseException]], exc_val: Optional[BaseException], exc_tb: Optional[TracebackType]) -> None: -- 2.34.1
[PATCH v2 4/9] python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
Copy the docstrings out of qemu.qmp, adjusting them as necessary to
more accurately reflect the current state of this class.
(Licensing: This is copying and modifying GPLv2-only licensed docstrings
into a GPLv2-only file.)
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
---
python/qemu/aqmp/legacy.py | 98 ++
1 file changed, 90 insertions(+), 8 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 10c7c99c4f0..dfcd20bbd23 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -1,7 +1,13 @@
"""
-Sync QMP Wrapper
+(Legacy) Sync QMP Wrapper
-This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+This module provides the `QEMUMonitorProtocol` class, which is a
+synchronous wrapper around `QMPClient`.
+
+Its design closely resembles that of the original QEMUMonitorProtocol
+class, originally written by Luiz Capitulino. It is provided here for
+compatibility with scripts inside the QEMU source tree that expect the
+old interface.
"""
#
@@ -50,9 +56,6 @@
# {} is the QMPReturnValue.
-# pylint: disable=missing-docstring
-
-
class QMPBadPortError(QMPError):
"""
Unable to parse socket address: Port was non-numerical.
@@ -60,6 +63,17 @@ class QMPBadPortError(QMPError):
class QEMUMonitorProtocol:
+"""
+Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP)
+and then allow to handle commands and events.
+
+:param address: QEMU address, can be either a unix socket path (string)
+ or a tuple in the form ( address, port ) for a TCP
+ connection
+:param server: Act as the socket server. (See 'accept')
+:param nickname: Optional nickname used for logging.
+"""
+
def __init__(self, address: SocketAddrT,
server: bool = False,
nickname: Optional[str] = None):
@@ -121,6 +135,12 @@ def parse_address(cls, address: str) -> SocketAddrT:
return address
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+"""
+Connect to the QMP Monitor and perform capabilities negotiation.
+
+:return: QMP greeting dict, or None if negotiate is false
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = negotiate
self._aqmp.negotiate = negotiate
@@ -130,6 +150,16 @@ def connect(self, negotiate: bool = True) ->
Optional[QMPMessage]:
return self._get_greeting()
def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+"""
+Await connection from QMP Monitor and perform capabilities negotiation.
+
+:param timeout:
+timeout in seconds (nonnegative float number, or None).
+If None, there is no timeout, and this may block forever.
+
+:return: QMP greeting dict
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = True
self._aqmp.negotiate = True
@@ -140,6 +170,12 @@ def accept(self, timeout: Optional[float] = 15.0) ->
QMPMessage:
return ret
def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+"""
+Send a QMP command to the QMP Monitor.
+
+:param qmp_cmd: QMP command to be sent as a Python dict
+:return: QMP response as a Python dict
+"""
return dict(
self._sync(
# pylint: disable=protected-access
@@ -158,9 +194,9 @@ def cmd(self, name: str,
"""
Build a QMP command and send it to the QMP Monitor.
-@param name: command name (string)
-@param args: command arguments (dict)
-@param cmd_id: command id (dict, list, string or int)
+:param name: command name (string)
+:param args: command arguments (dict)
+:param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd: QMPMessage = {'execute': name}
if args:
@@ -170,6 +206,9 @@ def cmd(self, name: str,
return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
return self._sync(
self._aqmp.execute(cmd, kwds),
self._timeout
@@ -177,6 +216,19 @@ def command(self, cmd: str, **kwds: object) ->
QMPReturnValue:
def pull_event(self,
wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+"""
+Pulls a single event.
+
+:param wait:
+If False or 0, do not wait. Retu
[PATCH v2 3/9] python/aqmp: fully separate from qmp.QEMUMonitorProtocol
After this patch, qemu.aqmp.legacy.QEMUMonitorProtocol no longer
inherits from qemu.qmp.QEMUMonitorProtocol. To do this, several
inherited methods need to be explicitly re-defined.
(Licensing: This is copying and modifying GPLv2-only code into a
GPLv2-only file.)
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
---
python/qemu/aqmp/legacy.py | 37 +++--
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index f0262749491..10c7c99c4f0 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -16,18 +16,18 @@
#
import asyncio
+from types import TracebackType
from typing import (
Any,
Awaitable,
Dict,
List,
Optional,
+Type,
TypeVar,
Union,
)
-import qemu.qmp
-
from .error import QMPError
from .protocol import Runstate, SocketAddrT
from .qmp_client import QMPClient
@@ -59,12 +59,11 @@ class QMPBadPortError(QMPError):
"""
-class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+class QEMUMonitorProtocol:
def __init__(self, address: SocketAddrT,
server: bool = False,
nickname: Optional[str] = None):
-# pylint: disable=super-init-not-called
self._aqmp = QMPClient(nickname)
self._aloop = asyncio.get_event_loop()
self._address = address
@@ -88,7 +87,18 @@ def _get_greeting(self) -> Optional[QMPMessage]:
return self._aqmp.greeting._asdict()
return None
-# __enter__ and __exit__ need no changes
+def __enter__(self: _T) -> _T:
+# Implement context manager enter function.
+return self
+
+def __exit__(self,
+ # pylint: disable=duplicate-code
+ # see https://github.com/PyCQA/pylint/issues/3619
+ exc_type: Optional[Type[BaseException]],
+ exc_val: Optional[BaseException],
+ exc_tb: Optional[TracebackType]) -> None:
+# Implement context manager exit function.
+self.close()
@classmethod
def parse_address(cls, address: str) -> SocketAddrT:
@@ -142,7 +152,22 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
)
)
-# Default impl of cmd() delegates to cmd_obj
+def cmd(self, name: str,
+args: Optional[Dict[str, object]] = None,
+cmd_id: Optional[object] = None) -> QMPMessage:
+"""
+Build a QMP command and send it to the QMP Monitor.
+
+@param name: command name (string)
+@param args: command arguments (dict)
+@param cmd_id: command id (dict, list, string or int)
+"""
+qmp_cmd: QMPMessage = {'execute': name}
+if args:
+qmp_cmd['arguments'] = args
+if cmd_id:
+qmp_cmd['id'] = cmd_id
+return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
return self._sync(
--
2.34.1
[PATCH v2 0/9] Python: Remove synchronous QMP library
Based-on: https://gitlab.com/jsnow/qemu/-/tree/python/
GitLab: https://gitlab.com/jsnow/qemu/-/tree/python-qmp-legacy-switch-pt1c
CI: https://gitlab.com/jsnow/qemu/-/pipelines/505169095
Hi, this series is part of an effort to publish the qemu.qmp package on
PyPI. It is the first of three series to complete this work:
--> (1) Switch the new async QMP library in as python/qemu/qmp
(2) Fork python/qemu/qmp out into its own repository,
with updated GitLab CI/CD targets to build packages.
(3) Update qemu.git to install qemu.qmp from PyPI,
and then delete python/qemu/qmp.
This series finalizes swapping out the old QMP library for the new
one. This leaves us with just one QMP library to worry about. It also
implements the rename of "qemu.aqmp" to "qemu.qmp".
This is the last patch series before I perform the actual fork.
These patches are (mostly) reviewed, so I'll likely stage these fairly
quickly barring any objections. The plan is to submit them as soon as
the tree re-opens to help prevent rot while I work on the fork.
John Snow (9):
python: temporarily silence pylint duplicate-code warnings
python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
python/aqmp: fully separate from qmp.QEMUMonitorProtocol
python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
python: remove the old QMP package
python: re-enable pylint duplicate-code warnings
python: rename qemu.aqmp to qemu.qmp
python: rename 'aqmp-tui' to 'qmp-tui'
python/qmp: remove pylint workaround from legacy.py
python/README.rst | 2 +-
python/qemu/qmp/README.rst| 9 -
python/qemu/aqmp/__init__.py | 59 ---
python/qemu/aqmp/legacy.py| 188
python/qemu/aqmp/py.typed | 0
python/qemu/machine/machine.py| 4 +-
python/qemu/machine/qtest.py | 2 +-
python/qemu/qmp/__init__.py | 445 ++
python/qemu/{aqmp => qmp}/error.py| 0
python/qemu/{aqmp => qmp}/events.py | 2 +-
python/qemu/qmp/legacy.py | 315 +
python/qemu/{aqmp => qmp}/message.py | 0
python/qemu/{aqmp => qmp}/models.py | 0
python/qemu/{aqmp => qmp}/protocol.py | 4 +-
python/qemu/{aqmp => qmp}/qmp_client.py | 16 +-
python/qemu/{aqmp => qmp}/qmp_shell.py| 4 +-
.../qemu/{aqmp/aqmp_tui.py => qmp/qmp_tui.py} | 15 +-
python/qemu/{aqmp => qmp}/util.py | 0
python/qemu/utils/qemu_ga_client.py | 4 +-
python/qemu/utils/qom.py | 2 +-
python/qemu/utils/qom_common.py | 4 +-
python/qemu/utils/qom_fuse.py | 2 +-
python/setup.cfg | 11 +-
python/tests/protocol.py | 14 +-
scripts/cpu-x86-uarch-abi.py | 2 +-
scripts/device-crash-test | 4 +-
scripts/qmp/qmp-shell | 2 +-
scripts/qmp/qmp-shell-wrap| 2 +-
scripts/render_block_graph.py | 4 +-
scripts/simplebench/bench_block_job.py| 2 +-
tests/qemu-iotests/iotests.py | 2 +-
tests/qemu-iotests/tests/mirror-top-perms | 4 +-
32 files changed, 409 insertions(+), 715 deletions(-)
delete mode 100644 python/qemu/qmp/README.rst
delete mode 100644 python/qemu/aqmp/__init__.py
delete mode 100644 python/qemu/aqmp/legacy.py
delete mode 100644 python/qemu/aqmp/py.typed
rename python/qemu/{aqmp => qmp}/error.py (100%)
rename python/qemu/{aqmp => qmp}/events.py (99%)
create mode 100644 python/qemu/qmp/legacy.py
rename python/qemu/{aqmp => qmp}/message.py (100%)
rename python/qemu/{aqmp => qmp}/models.py (100%)
rename python/qemu/{aqmp => qmp}/protocol.py (99%)
rename python/qemu/{aqmp => qmp}/qmp_client.py (97%)
rename python/qemu/{aqmp => qmp}/qmp_shell.py (99%)
rename python/qemu/{aqmp/aqmp_tui.py => qmp/qmp_tui.py} (98%)
rename python/qemu/{aqmp => qmp}/util.py (100%)
--
2.34.1
[PATCH v2 5/9] python: remove the old QMP package
Thank you for your service!
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Beraldo Leal
---
python/PACKAGE.rst | 4 +-
python/README.rst | 2 +-
python/qemu/qmp/README.rst | 9 -
python/qemu/qmp/__init__.py | 396
python/qemu/qmp/py.typed| 0
python/setup.cfg| 3 +-
6 files changed, 4 insertions(+), 410 deletions(-)
delete mode 100644 python/qemu/qmp/README.rst
delete mode 100644 python/qemu/qmp/__init__.py
delete mode 100644 python/qemu/qmp/py.typed
diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index b0b86cc4c31..ddfa9ba3f59 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
Usage
-
-The ``qemu.qmp`` subpackage provides a library for communicating with
+The ``qemu.aqmp`` subpackage provides a library for communicating with
QMP servers. The ``qemu.machine`` subpackage offers rudimentary
facilities for launching and managing QEMU processes. Refer to each
package's documentation
-(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
for more information.
Contributing
diff --git a/python/README.rst b/python/README.rst
index fcf74f69eae..eb5213337d2 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
This directory houses Python tooling used by the QEMU project to build,
configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
``setup.py`` is used by ``pip`` to install this tooling to the current
environment. ``setup.cfg`` provides the packaging configuration used by
diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
deleted file mode 100644
index 5bfb82535f8..000
--- a/python/qemu/qmp/README.rst
+++ /dev/null
@@ -1,9 +0,0 @@
-qemu.qmp package
-
-
-This package provides a library used for connecting to and communicating
-with QMP servers. It is used extensively by iotests, vm tests,
-avocado tests, and other utilities in the ./scripts directory. It is
-not a fully-fledged SDK and is subject to change at any time.
-
-See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
deleted file mode 100644
index 4e086411544..000
--- a/python/qemu/qmp/__init__.py
+++ /dev/null
@@ -1,396 +0,0 @@
-"""
-QEMU Monitor Protocol (QMP) development library & tooling.
-
-This package provides a fairly low-level class for communicating to QMP
-protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
-QEMU Storage Daemon. This library is not intended for production use.
-
-`QEMUMonitorProtocol` is the primary class of interest, and all errors
-raised derive from `QMPError`.
-"""
-
-# Copyright (C) 2009, 2010 Red Hat Inc.
-#
-# Authors:
-# Luiz Capitulino
-#
-# This work is licensed under the terms of the GNU GPL, version 2. See
-# the COPYING file in the top-level directory.
-
-import errno
-import json
-import logging
-import socket
-import struct
-from types import TracebackType
-from typing import (
-Any,
-Dict,
-List,
-Optional,
-TextIO,
-Tuple,
-Type,
-TypeVar,
-Union,
-cast,
-)
-
-
-#: QMPMessage is an entire QMP message of any kind.
-QMPMessage = Dict[str, Any]
-
-#: QMPReturnValue is the 'return' value of a command.
-QMPReturnValue = object
-
-#: QMPObject is any object in a QMP message.
-QMPObject = Dict[str, object]
-
-# QMPMessage can be outgoing commands or incoming events/returns.
-# QMPReturnValue is usually a dict/json object, but due to QAPI's
-# 'returns-whitelist', it can actually be anything.
-#
-# {'return': {}} is a QMPMessage,
-# {} is the QMPReturnValue.
-
-
-InternetAddrT = Tuple[str, int]
-UnixAddrT = str
-SocketAddrT = Union[InternetAddrT, UnixAddrT]
-
-
-class QMPError(Exception):
-"""
-QMP base exception
-"""
-
-
-class QMPConnectError(QMPError):
-"""
-QMP connection exception
-"""
-
-
-class QMPCapabilitiesError(QMPError):
-"""
-QMP negotiate capabilities exception
-"""
-
-
-class QMPTimeoutError(QMPError):
-"""
-QMP timeout exception
-"""
-
-
-class QMPProtocolError(QMPError):
-"""
-QMP protocol error; unexpected response
-"""
-
-
-class QMPResponseError(QMPError):
-"""
-Represents erroneous QMP monitor reply
-"""
-def __init__(self, reply: QMPMessage):
-try:
-desc = reply['error']['desc']
-
[PATCH v2 7/9] python: rename qemu.aqmp to qemu.qmp
Now that we are fully switched over to the new QMP library, move it back
over the old namespace. This is being done primarily so that we may
upload this package simply as "qemu.qmp" without introducing confusion
over whether or not "aqmp" is a new protocol or not.
The trade-off is increased confusion inside the QEMU developer
tree. Sorry!
Note: the 'private' member "_aqmp" in legacy.py also changes to "_qmp";
not out of necessity, but just to remove any traces of the "aqmp"
name.
Signed-off-by: John Snow
Reviewed-by: Beraldo Leal
Acked-by: Hanna Reitz
---
python/PACKAGE.rst| 4 +--
python/README.rst | 4 +--
python/qemu/machine/machine.py| 4 +--
python/qemu/machine/qtest.py | 2 +-
python/qemu/{aqmp => qmp}/__init__.py | 6 ++--
python/qemu/{aqmp => qmp}/aqmp_tui.py | 0
python/qemu/{aqmp => qmp}/error.py| 0
python/qemu/{aqmp => qmp}/events.py | 2 +-
python/qemu/{aqmp => qmp}/legacy.py | 38 +++
python/qemu/{aqmp => qmp}/message.py | 0
python/qemu/{aqmp => qmp}/models.py | 0
python/qemu/{aqmp => qmp}/protocol.py | 4 +--
python/qemu/{aqmp => qmp}/py.typed| 0
python/qemu/{aqmp => qmp}/qmp_client.py | 16 +-
python/qemu/{aqmp => qmp}/qmp_shell.py| 4 +--
python/qemu/{aqmp => qmp}/util.py | 0
python/qemu/utils/qemu_ga_client.py | 4 +--
python/qemu/utils/qom.py | 2 +-
python/qemu/utils/qom_common.py | 4 +--
python/qemu/utils/qom_fuse.py | 2 +-
python/setup.cfg | 10 +++---
python/tests/protocol.py | 14 -
scripts/cpu-x86-uarch-abi.py | 2 +-
scripts/device-crash-test | 4 +--
scripts/qmp/qmp-shell | 2 +-
scripts/qmp/qmp-shell-wrap| 2 +-
scripts/render_block_graph.py | 4 +--
scripts/simplebench/bench_block_job.py| 2 +-
tests/qemu-iotests/iotests.py | 2 +-
tests/qemu-iotests/tests/mirror-top-perms | 4 +--
30 files changed, 71 insertions(+), 71 deletions(-)
rename python/qemu/{aqmp => qmp}/__init__.py (87%)
rename python/qemu/{aqmp => qmp}/aqmp_tui.py (100%)
rename python/qemu/{aqmp => qmp}/error.py (100%)
rename python/qemu/{aqmp => qmp}/events.py (99%)
rename python/qemu/{aqmp => qmp}/legacy.py (91%)
rename python/qemu/{aqmp => qmp}/message.py (100%)
rename python/qemu/{aqmp => qmp}/models.py (100%)
rename python/qemu/{aqmp => qmp}/protocol.py (99%)
rename python/qemu/{aqmp => qmp}/py.typed (100%)
rename python/qemu/{aqmp => qmp}/qmp_client.py (97%)
rename python/qemu/{aqmp => qmp}/qmp_shell.py (99%)
rename python/qemu/{aqmp => qmp}/util.py (100%)
diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index ddfa9ba3f59..b0b86cc4c31 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
Usage
-
-The ``qemu.aqmp`` subpackage provides a library for communicating with
+The ``qemu.qmp`` subpackage provides a library for communicating with
QMP servers. The ``qemu.machine`` subpackage offers rudimentary
facilities for launching and managing QEMU processes. Refer to each
package's documentation
-(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
for more information.
Contributing
diff --git a/python/README.rst b/python/README.rst
index eb5213337d2..9c1fceaee73 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
This directory houses Python tooling used by the QEMU project to build,
configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
``setup.py`` is used by ``pip`` to install this tooling to the current
environment. ``setup.cfg`` provides the packaging configuration used by
@@ -59,7 +59,7 @@ Package installation also normally provides executable
console scripts,
so that tools like ``qmp-shell`` are always available via $PATH. To
invoke them without installation, you can invoke e.g.:
-``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
+``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
The mappings between console script name and python module path can be
found in ``setup.cfg``.
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 41be025ac7b..07ac5a710be 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,8 +40,8 @@
TypeVar,
)
-from qemu.aqmp import SocketAddrT
-from qemu.aqmp.legacy import (
+from qemu.qmp import Socket
[PATCH v2 6/9] python: re-enable pylint duplicate-code warnings
With the old library gone, there's nothing duplicated in the tree, so the warning suppression can be removed. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Beraldo Leal --- python/setup.cfg | 1 - 1 file changed, 1 deletion(-) diff --git a/python/setup.cfg b/python/setup.cfg index 4340c29a24f..49e3c285f19 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -118,7 +118,6 @@ disable=consider-using-f-string, too-many-function-args, # mypy handles this with less false positives. too-many-instance-attributes, no-member, # mypy also handles this better. -duplicate-code, # To be removed by the end of this patch series. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.34.1
