Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507
On Fri, Mar 18, 2022 at 2:50 PM Thomas Huth wrote: > > On 10/03/2022 18.53, Jon Maloy wrote: > > > > On 3/10/22 12:14, Thomas Huth wrote: > >> On 06/02/2022 20.19, Jon Maloy wrote: > >>> Trying again with correct email address. > >>> ///jon > >>> > >>> On 2/6/22 14:15, Jon Maloy wrote: > > > On 1/27/22 15:14, Jon Maloy wrote: > > > > On 11/18/21 06:57, Philippe Mathieu-Daudé wrote: > >> Trivial fix for CVE-2021-3507. > >> > >> Philippe Mathieu-Daudé (2): > >>hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507) > >>tests/qtest/fdc-test: Add a regression test for CVE-2021-3507 > >> > >> hw/block/fdc.c | 8 > >> tests/qtest/fdc-test.c | 20 > >> 2 files changed, 28 insertions(+) > >> > > Series > > Acked-by: Jon Maloy > > Philippe, > I hear from other sources that you earlier have qualified this one as > "incomplete". > I am of course aware that this one, just like my own patch, is just a > mitigation and not a complete correction of the erroneous calculation. > Or did you have anything else in mind? > >> > >> Any news on this one? It would be nice to get the CVE fixed for 7.0 ? > >> > >> Thomas > >> > > The ball is currently with John Snow, as I understand it. > > The concern is that this fix may not take the driver back to a consistent > > state, so that we may have other problems later. > > Maybe Philippe can chip in with a comment here? > > John, Philippe, any ideas how to move this forward? > > Thomas > The ball is indeed in my court. I need to audit this properly and get the patch re-applied, and get tests passing. As a personal favor: Could you please ping me on IRC tomorrow about this? (Well, later today, for you.)
Re: [PATCH v2 1/3] qapi: rename BlockDirtyBitmapMergeSource to BlockDirtyBitmapOrStr
On Wed, Mar 16, 2022 at 5:18 PM Eric Blake wrote: > > On Tue, Mar 15, 2022 at 12:32:24AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > From: Vladimir Sementsov-Ogievskiy > > > > Rename the type to be reused. Old name is "what is it for". To be > > natively reused for other needs, let's name it exactly "what is it". > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > --- > > block/monitor/bitmap-qmp-cmds.c| 6 +++--- > > include/block/block_int-global-state.h | 2 +- > > qapi/block-core.json | 6 +++--- > > qemu-img.c | 8 > > 4 files changed, 11 insertions(+), 11 deletions(-) > > The type name does not affect QAPI interface stability, so this is safe. > > Reviewed-by: Eric Blake Sounds right. Acked-by: John Snow
Re: [PULL 0/8] Fix CVE-2021-3611 and heap overflow in sdhci code
On Mon, 21 Mar 2022 at 17:03, Thomas Huth wrote: > > The following changes since commit 2058fdbe81e2985c226a026851dd26b146d3395c: > > Merge tag 'fixes-20220318-pull-request' of git://git.kraxel.org/qemu into > staging (2022-03-19 11:28:54 +) > > are available in the Git repository at: > > https://gitlab.com/thuth/qemu.git tags/pull-request-2022-03-21 > > for you to fetch changes up to 27801168ecbb34b987d2e92a12369367bf9ac2bf: > > tests/qtest/fuzz-sdcard-test: Add reproducer for OSS-Fuzz (Issue 29225) > (2022-03-21 14:05:42 +0100) > > > * Fix stack-overflow due to recursive DMA in intel-hda (CVE-2021-3611) > * Fix heap overflow due to recursive DMA in sdhci code > > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/7.0 for any user-visible changes. -- PMM
Re: [PATCH 13/15] iotests: remove qemu_io_pipe_and_status()
On Tue, Mar 22, 2022 at 12:39 PM Hanna Reitz wrote: > > On 18.03.22 21:36, John Snow wrote: > > 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 > > --- > > tests/qemu-iotests/iotests.py | 3 --- > > tests/qemu-iotests/tests/image-fleecing | 12 +++- > > 2 files changed, 3 insertions(+), 12 deletions(-) > > Reviewed-by: Hanna Reitz I goofed this patch -- some of the failures in this test are expected and this patch breaks the test. Dropping the R-Bs. v2 will explain what's up in the commit message. --js
[PATCH] iotests: update test owner contact information
Quite a few of these tests have stale contact information. This patch updates the stale ones that I happen to be aware of at the moment. Signed-off-by: John Snow --- tests/qemu-iotests/025 | 2 +- tests/qemu-iotests/027 | 2 +- tests/qemu-iotests/028 | 2 +- tests/qemu-iotests/036 | 2 +- tests/qemu-iotests/039 | 2 +- tests/qemu-iotests/059 | 2 +- tests/qemu-iotests/060 | 2 +- tests/qemu-iotests/061 | 2 +- tests/qemu-iotests/062 | 2 +- tests/qemu-iotests/064 | 2 +- tests/qemu-iotests/066 | 2 +- tests/qemu-iotests/068 | 2 +- tests/qemu-iotests/069 | 2 +- tests/qemu-iotests/070 | 2 +- tests/qemu-iotests/071 | 2 +- tests/qemu-iotests/072 | 2 +- tests/qemu-iotests/074 | 2 +- tests/qemu-iotests/084 | 2 +- tests/qemu-iotests/085 | 2 +- tests/qemu-iotests/089 | 2 +- tests/qemu-iotests/090 | 2 +- tests/qemu-iotests/091 | 2 +- tests/qemu-iotests/094 | 2 +- tests/qemu-iotests/095 | 2 +- tests/qemu-iotests/097 | 2 +- tests/qemu-iotests/098 | 2 +- tests/qemu-iotests/099 | 2 +- tests/qemu-iotests/102 | 2 +- tests/qemu-iotests/103 | 2 +- tests/qemu-iotests/105 | 2 +- tests/qemu-iotests/106 | 2 +- tests/qemu-iotests/107 | 2 +- tests/qemu-iotests/108 | 2 +- tests/qemu-iotests/110 | 2 +- tests/qemu-iotests/111 | 2 +- tests/qemu-iotests/112 | 2 +- tests/qemu-iotests/113 | 2 +- tests/qemu-iotests/115 | 2 +- tests/qemu-iotests/117 | 2 +- tests/qemu-iotests/119 | 2 +- tests/qemu-iotests/120 | 2 +- tests/qemu-iotests/121 | 2 +- tests/qemu-iotests/123 | 2 +- tests/qemu-iotests/125 | 2 +- tests/qemu-iotests/126 | 2 +- tests/qemu-iotests/127 | 2 +- tests/qemu-iotests/135 | 2 +- tests/qemu-iotests/138 | 2 +- tests/qemu-iotests/140 | 2 +- tests/qemu-iotests/141 | 2 +- tests/qemu-iotests/143 | 2 +- tests/qemu-iotests/144 | 2 +- tests/qemu-iotests/146 | 2 +- tests/qemu-iotests/150 | 2 +- tests/qemu-iotests/153 | 2 +- tests/qemu-iotests/156 | 2 +- tests/qemu-iotests/162 | 2 +- tests/qemu-iotests/173 | 2 +- tests/qemu-iotests/176 | 2 +- tests/qemu-iotests/182 | 2 +- tests/qemu-iotests/192 | 2 +- tests/qemu-iotests/200 | 2 +- tests/qemu-iotests/216 | 2 +- tests/qemu-iotests/218 | 2 +- tests/qemu-iotests/224 | 2 +- tests/qemu-iotests/225 | 2 +- tests/qemu-iotests/228 | 2 +- tests/qemu-iotests/229 | 2 +- tests/qemu-iotests/231 | 2 +- tests/qemu-iotests/250 | 2 +- tests/qemu-iotests/251 | 2 +- tests/qemu-iotests/252 | 2 +- tests/qemu-iotests/258 | 2 +- tests/qemu-iotests/259 | 2 +- tests/qemu-iotests/261 | 2 +- tests/qemu-iotests/310 | 2 +- 76 files changed, 76 insertions(+), 76 deletions(-) diff --git a/tests/qemu-iotests/025 b/tests/qemu-iotests/025 index 80686a30d5..5771ea9200 100755 --- a/tests/qemu-iotests/025 +++ b/tests/qemu-iotests/025 @@ -20,7 +20,7 @@ # # creator -owner=stefa...@linux.vnet.ibm.com +owner=stefa...@redhat.com seq=`basename $0` echo "QA output created by $seq" diff --git a/tests/qemu-iotests/027 b/tests/qemu-iotests/027 index b279c88f33..24c93627bb 100755 --- a/tests/qemu-iotests/027 +++ b/tests/qemu-iotests/027 @@ -20,7 +20,7 @@ # # creator -owner=stefa...@linux.vnet.ibm.com +owner=stefa...@redhat.com seq=`basename $0` echo "QA output created by $seq" diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028 index 8c391f2adc..2b232c4614 100755 --- a/tests/qemu-iotests/028 +++ b/tests/qemu-iotests/028 @@ -23,7 +23,7 @@ # # creator -owner=stefa...@linux.vnet.ibm.com +owner=stefa...@redhat.com seq=`basename $0` echo "QA output created by $seq" diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036 index f703605e44..16a401985c 100755 --- a/tests/qemu-iotests/036 +++ b/tests/qemu-iotests/036 @@ -23,7 +23,7 @@ # # creator -owner=stefa...@linux.vnet.ibm.com +owner=stefa...@redhat.com seq=`basename $0` echo "QA output created by $seq" diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 index 00d379cde2..e43e7026ce 100755 --- a/tests/qemu-iotests/039 +++ b/tests/qemu-iotests/039 @@ -23,7 +23,7 @@ # # creator -owner=stefa...@linux.vnet.ibm.com +owner=stefa...@redhat.com seq=`basename $0` echo "QA output created by $seq" diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059 index 65c0c32b26..e8be217e1f 100755 --- a/tests/qemu-iotests/059 +++ b/tests/qemu-iotests/059 @@ -20,7 +20,7 @@ # # creator -owner=f...@redhat.com +owner=f...@euphon.net seq=`basename $0` echo "QA output created by $seq" diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060 index df87d600f7..5cd21a6f68 100755 --- a/tests/qemu-iotests/060 +++ b/tests/qemu-iotests/060 @@ -20,7 +20,7 @@ # # creator -owner=mre...@redhat.com +owner=hre...@redhat.com seq="$(basename $0)" echo "QA output created by $seq" diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061 index 513fbec14c..509ad247cd 100755 --- a/tests/qemu-iotests/061 +++ b/tests/qemu-iotests/061 @@ -20,7 +20,7 @@ # # creator -owner=mre...@redhat.com +owner=hre...@redhat.com seq=`basename $0` echo "QA output created by $seq" diff --git
Re: [PATCH v2 3/3] Use g_new() & friends where that makes obvious sense
On Mar 15 15:41, Markus Armbruster wrote: > g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, > for two reasons. One, it catches multiplication overflowing size_t. > Two, it returns T * rather than void *, which lets the compiler catch > more type errors. > > This commit only touches allocations with size arguments of the form > sizeof(T). > > Patch created mechanically with: > > $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ >--macro-file scripts/cocci-macro-file.h FILES... > > Signed-off-by: Markus Armbruster > Reviewed-by: Philippe Mathieu-Daudé > Reviewed-by: Cédric Le Goater > Reviewed-by: Alex Bennée > Acked-by: Dr. David Alan Gilbert > --- > hw/nvme/ns.c | 2 +- For hw/nvme, Acked-by: Klaus Jensen signature.asc Description: PGP signature
Re: [PATCH 00/15] iotests: add enhanced debugging info to qemu-io failures
On 18.03.22 21:36, John Snow wrote:
Howdy,
Heya,
[...]
- Uh, actually, test 040 fails with this patchset and I don't understand
if it's intentional, harmless, a test design problem, or worse:
==
ERROR: test_filterless_commit (__main__.TestCommitWithFilters)
--
Traceback (most recent call last):
File "/home/jsnow/src/qemu/tests/qemu-iotests/040", line 822, in tearDown
self.do_test_io('read')
File "/home/jsnow/src/qemu/tests/qemu-iotests/040", line 751, in do_test_io
qemu_io('-f', iotests.imgfmt,
File "/home/jsnow/src/qemu/tests/qemu-iotests/iotests.py", line 365, in
qemu_io
return qemu_tool(*qemu_io_wrap_args(args),
File "/home/jsnow/src/qemu/tests/qemu-iotests/iotests.py", line 242, in
qemu_tool
raise VerboseProcessError(
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
┗━
It looks like we start with the img chain 3->2->1->0, then we commit 2
down into 1, but checking '3' fails somewhere in the backing
chain. Maybe a real bug?
Looks like my hunch was right: The problem is that it’s hard to figure
out a good backing file string when there are filters involved, and so
in one test here we generate one that contains a JSON description of the
backing subgraph including a throttle node. Outside of qemu, that
doesn’t make much sense, hence the error.
(And because we checked only for “pattern verification failed”
specifically, that error here never surfaced.)
I think (hope?) we can expect management tools to manually specify
backing file strings in such cases, like the attached diff does. That
seems to fix the problem.
Hannadiff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index c4a90937dc..30eb97829e 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')
Re: [PATCH 14/15] iotests: remove qemu_io_silent() and qemu_io_silent_check().
On Tue, Mar 22, 2022 at 1:00 PM Hanna Reitz wrote: > > On 18.03.22 21:36, John Snow wrote: > > 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. > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/216| 12 +- > > tests/qemu-iotests/218| 5 ++--- > > tests/qemu-iotests/224| 4 ++-- > > tests/qemu-iotests/258| 12 +- > > tests/qemu-iotests/298| 16 ++ > > 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, 39 insertions(+), 58 deletions(-) > > qemu_io_silent_check() was unused anyway, right...? > > Reviewed-by: Hanna Reitz > As far as I can tell.
Re: [PATCH 04/15] iotests/040: Don't check image pattern on zero-length image
On 22.03.22 17:19, John Snow wrote: On Tue, Mar 22, 2022, 10:22 AM Hanna Reitz wrote: On 18.03.22 21:36, John Snow wrote: > qemu-io fails on read/write with zero-length raw images, so skip these > when running the zero-length image tests. > > Signed-off-by: John Snow > --- > tests/qemu-iotests/040 | 14 -- > 1 file changed, 12 insertions(+), 2 deletions(-) Doesn’t look specific to zero-length images, but the fact that we do I/O beyond the image size, i.e. any image below 1 MB would be affected. Anyway, the zero-length image is the only one tested with a size of less than 1 MB, so this works. Reviewed-by: Hanna Reitz (Check the cover letter, too - this patch is still good, but iotest 040 still fails and I'm not 100% clear as to why.) Sure, but I didn’t see anything wrong with the patch, so... From a glance, I believe the problem to be that the commit job changed one image’s backing file string to contain a JSON description of a block graph including a throttle node. The accompanying throttle group of course doesn’t exist outside of qemu, so qemu-io complains. We never noticed because we just checked for “pattern verification failed”, which isn’t in the error message, so this was a pass. I’ll take a closer look. Hanna
Re: [PATCH 12/15] iotests/migration-permissions: use assertRaises() for qemu_io() negative test
On Tue, Mar 22, 2022 at 12:37 PM Hanna Reitz wrote: > > On 18.03.22 21:36, John Snow wrote: > > 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. > > > > Signed-off-by: John Snow > > --- > > .../qemu-iotests/tests/migration-permissions | 28 +-- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > Just like Eric I don’t find it so tidy that `ctx` exists outside of the > `with` block, but re-raising the exception is indeed better, so: > > Reviewed-by: Hanna Reitz > "Sorry about Python." You still have the object, but __exit__() will have been called, so resources will have been freed, references to live objects severed, etc. unittests is designed around this for Exception testing, etc. It's not a *real* scope. It just looks like one. I'll add a little note in the commit message for the next person who wonders. --js
Re: [PATCH 15/15] iotests: make qemu_io_log() check return codes by default
On 18.03.22 21:36, John Snow wrote: Just like qemu_img_log(), upgrade qemu_io_log() to enforce a return code of zero by default. Affected tests: 242 245 255 274 303 307 nbd-reconnect-on-open Signed-off-by: John Snow --- tests/qemu-iotests/iotests.py | 5 +++-- tests/qemu-iotests/tests/nbd-reconnect-on-open | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) Reviewed-by: Hanna Reitz
Re: [PULL 00/25] Block patches for 7.0-rc1
On Tue, 22 Mar 2022 at 11:56, Hanna Reitz wrote: > > The following changes since commit 330724977b10f5b92610817e8b7d1dfed122df87: > > Merge tag 'pull-misc-2022-03-21' of git://repo.or.cz/qemu/armbru into > staging (2022-03-21 17:46:40 +) > > are available in the Git repository at: > > https://gitlab.com/hreitz/qemu.git tags/pull-block-2022-03-22 > > for you to fetch changes up to 48f1fcd5c87cadef331a2cd08e67e37a789e8347: > > iotests/207: Filter host fingerprint (2022-03-22 10:50:10 +0100) > > > Block patches for 7.0-rc1: > - iotest fixes: > - Fix some iotests for riscv targets > - Use GNU sed in more places where required > - Meson-related fixes (i.e. to print errors when they occur) > - Have qemu-img calls (from Python tests) generally raise nicely > formattable exceptions on errors > - Fix iotest 207 > - Allow RBD images to be growable by writing zeroes past the end of > file, fixing qcow2 on rbd > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/7.0 for any user-visible changes. -- PMM
Re: [PATCH 14/15] iotests: remove qemu_io_silent() and qemu_io_silent_check().
On 18.03.22 21:36, John Snow wrote: 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. Signed-off-by: John Snow --- tests/qemu-iotests/216| 12 +- tests/qemu-iotests/218| 5 ++--- tests/qemu-iotests/224| 4 ++-- tests/qemu-iotests/258| 12 +- tests/qemu-iotests/298| 16 ++ 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, 39 insertions(+), 58 deletions(-) qemu_io_silent_check() was unused anyway, right...? Reviewed-by: Hanna Reitz
Re: [PATCH 10/15] iotests/245: fixup
On Fri, Mar 18, 2022 at 4:37 PM John Snow wrote: > > (Merge with prior patch.) > > Signed-off-by: John Snow > --- > tests/qemu-iotests/242 | 2 +- ^ Oh, there's the stray import changes that needed to be folded into patch #1. Fixed for v2.
Re: [PATCH 13/15] iotests: remove qemu_io_pipe_and_status()
On 18.03.22 21:36, John Snow wrote: 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 --- tests/qemu-iotests/iotests.py | 3 --- tests/qemu-iotests/tests/image-fleecing | 12 +++- 2 files changed, 3 insertions(+), 12 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH 12/15] iotests/migration-permissions: use assertRaises() for qemu_io() negative test
On 18.03.22 21:36, John Snow wrote: 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. Signed-off-by: John Snow --- .../qemu-iotests/tests/migration-permissions | 28 +-- 1 file changed, 14 insertions(+), 14 deletions(-) Just like Eric I don’t find it so tidy that `ctx` exists outside of the `with` block, but re-raising the exception is indeed better, so: Reviewed-by: Hanna Reitz
Re: [PATCH 10/15] iotests/245: fixup
On 22.03.22 17:36, John Snow wrote:
On Tue, Mar 22, 2022, 12:30 PM Hanna Reitz wrote:
On 18.03.22 21:36, John Snow wrote:
> (Merge with prior patch.)
>
> Signed-off-by: John Snow
> ---
> tests/qemu-iotests/242 | 2 +-
> tests/qemu-iotests/245 | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
> index 4b7ec16af6..ecc851582a 100755
> --- a/tests/qemu-iotests/242
> +++ b/tests/qemu-iotests/242
> @@ -22,7 +22,7 @@
> import iotests
> import json
> import struct
> -from iotests import qemu_img_create, qemu_io, qemu_img_info, \
> +from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
> file_path, img_info_log, log, filter_qemu_io
>
> iotests.script_initialize(supported_fmts=['qcow2'],
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index 8cbed7821b..efdad1a0c4 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -217,7 +217,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
> # Reopen an image several times changing some of its options
> def test_reopen(self):
> # Check whether the filesystem supports O_DIRECT
> - if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none',
'-c', 'quit', hd_path[0]):
> + if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none',
'-c', 'quit', hd_path[0]).stdout:
This is to verify that O_DIRECT works or not. If it doesn’t work,
this
will fail, so we need to pass check=False here.
(Or this test fails on tmpfs.)
Hanna
Oh, I didn't realize a solitary "quit" command could still fail.
Thanks for the tip.
What’s being tested is the implicit `open`. :)
Hanna
Re: [PATCH 10/15] iotests/245: fixup
On Tue, Mar 22, 2022, 12:30 PM Hanna Reitz wrote:
> On 18.03.22 21:36, John Snow wrote:
> > (Merge with prior patch.)
> >
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/242 | 2 +-
> > tests/qemu-iotests/245 | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
> > index 4b7ec16af6..ecc851582a 100755
> > --- a/tests/qemu-iotests/242
> > +++ b/tests/qemu-iotests/242
> > @@ -22,7 +22,7 @@
> > import iotests
> > import json
> > import struct
> > -from iotests import qemu_img_create, qemu_io, qemu_img_info, \
> > +from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
> > file_path, img_info_log, log, filter_qemu_io
> >
> > iotests.script_initialize(supported_fmts=['qcow2'],
> > diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> > index 8cbed7821b..efdad1a0c4 100755
> > --- a/tests/qemu-iotests/245
> > +++ b/tests/qemu-iotests/245
> > @@ -217,7 +217,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
> > # Reopen an image several times changing some of its options
> > def test_reopen(self):
> > # Check whether the filesystem supports O_DIRECT
> > -if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c',
> 'quit', hd_path[0]):
> > +if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c',
> 'quit', hd_path[0]).stdout:
>
> This is to verify that O_DIRECT works or not. If it doesn’t work, this
> will fail, so we need to pass check=False here.
>
> (Or this test fails on tmpfs.)
>
> Hanna
>
Oh, I didn't realize a solitary "quit" command could still fail. Thanks for
the tip.
Re: [PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()
On Tue, Mar 22, 2022, 11:04 AM Hanna Reitz wrote:
> On 18.03.22 21:36, John Snow wrote:
> > 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
> >
> > Signed-off-by: John Snow
> >
> > ---
> >
> > Note: This breaks several tests at this point. I'll be fixing each
> > broken test one by one in the subsequent commits. We can squash them all
> > on merge to avoid test regressions.
>
> Well, absolutely.
>
> > (Seems like a way to have your cake and eat it too with regards to
> > maintaining bisectability while also having nice mailing list patches.)
>
> I personally find reviewability to not be affected whether this is one
> patch or multiple, given that the changes are in different files anyway.
>
> I am afraid someone might forgot to squash when merging this series,
> though...
>
> Also, I don’t know how to squash R-b tags, and I don’t feel like I can
> give an R-b for a patch that decidedly breaks tests.
>
> >
> > Copy-pastables:
> >
> > ./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 ones that fail:
> > ./check -qcow2 030 040 242 245
> > ./check -raw migration-permissions
> > ./check -nbd 205
> > ./check -luks 149
> >
> > Signed-off-by: John Snow
> > ---
> > tests/qemu-iotests/iotests.py | 19 +--
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index 974a2b0c8d..58ea766568 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -354,16 +354,23 @@ def qemu_io_wrap_args(args: Sequence[str]) ->
> List[str]:
> > def qemu_io_popen(*args):
> > return qemu_tool_popen(qemu_io_wrap_args(args))
> >
> > -def qemu_io(*args):
> > -'''Run qemu-io and return the stdout data'''
> > -return qemu_tool_pipe_and_status('qemu-io',
> qemu_io_wrap_args(args))[0]
> > +def qemu_io(*args: str, check: bool = True, combine_stdio: bool = True
> > +) -> subprocess.CompletedProcess[str]:
>
> I guess this return type probably has to be quoted.
>
Yep. Sent this just before I figured out the problem from the prior series.
I'll make sure this whole series passes CI before I send it out a second
time.
I'll rebase on your staging branch and take my time with v2.
> > +"""
> > +Run QEMU_IO_PROG and return the status code and console output.
> > +
> > +This function always prepends either QEMU_IO_OPTIONS or
> > +QEMU_IO_OPTIONS_NO_FMT.
> > +"""
> > +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):
> > -result = qemu_io(*args)
> > -log(result, filters=[filter_testfiles, filter_qemu_io])
> > +def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:
>
> ...and this one.
>
> Hanna
>
> > +result = qemu_io(*args, check=False)
> > +log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
> > return result
> >
> > def qemu_io_silent(*args):
>
>
Re: [PATCH 10/15] iotests/245: fixup
On 18.03.22 21:36, John Snow wrote:
(Merge with prior patch.)
Signed-off-by: John Snow
---
tests/qemu-iotests/242 | 2 +-
tests/qemu-iotests/245 | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 4b7ec16af6..ecc851582a 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -22,7 +22,7 @@
import iotests
import json
import struct
-from iotests import qemu_img_create, qemu_io, qemu_img_info, \
+from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
file_path, img_info_log, log, filter_qemu_io
iotests.script_initialize(supported_fmts=['qcow2'],
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 8cbed7821b..efdad1a0c4 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -217,7 +217,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# Reopen an image several times changing some of its options
def test_reopen(self):
# Check whether the filesystem supports O_DIRECT
-if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c', 'quit',
hd_path[0]):
+if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c', 'quit',
hd_path[0]).stdout:
This is to verify that O_DIRECT works or not. If it doesn’t work, this
will fail, so we need to pass check=False here.
(Or this test fails on tmpfs.)
Hanna
supports_direct = False
else:
supports_direct = True
Re: [PATCH 08/15] iotests/149: fixup
On 18.03.22 21:36, John Snow wrote:
(Merge into prior patch.)
Notes: I don't quite like this change, but I'm at a loss for what would
be cleaner. This is a funky test.
Signed-off-by: John Snow
---
tests/qemu-iotests/149 | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
I mean, looks fine to me, fwiw.
Hanna
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 9bb96d6a1d..2ae318f16f 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -295,7 +295,8 @@ def qemu_io_write_pattern(config, pattern, offset_mb,
size_mb, dev=False):
args = ["-c", "write -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
args.extend(qemu_io_image_args(config, dev))
iotests.log("qemu-io " + " ".join(args),
filters=[iotests.filter_test_dir])
-iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+output = iotests.qemu_io(*args, check=False).stdout
+iotests.log(check_cipher_support(config, output),
filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
@@ -307,7 +308,8 @@ def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
args = ["-c", "read -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
args.extend(qemu_io_image_args(config, dev))
iotests.log("qemu-io " + " ".join(args),
filters=[iotests.filter_test_dir])
-iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+output = iotests.qemu_io(*args, check=False).stdout
+iotests.log(check_cipher_support(config, output),
filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
Re: [PATCH 05/15] iotests: create generic qemu_tool() function
On Tue, Mar 22, 2022, 10:49 AM Hanna Reitz wrote: > On 18.03.22 21:36, John Snow wrote: > > reimplement qemu_img() in terms of qemu_tool() in preparation for doing > > the same with qemu_io(). > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/iotests.py | 37 +++ > > 1 file changed, 24 insertions(+), 13 deletions(-) > > > > diff --git a/tests/qemu-iotests/iotests.py > b/tests/qemu-iotests/iotests.py > > index 6cd8374c81..974a2b0c8d 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 > > + > > +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: command-line arguments to a QEMU cli tool. > > This makes me ask how I am to specify which tool to use. Perhaps it > should just be “full command line to run” or something. > > Might be nice™, but: > I see what you mean. I did away with the "tool name" parameter because we were only using it for an error message print I also removed. I'll update the docstring a little to make what's going on more clear. Maybe someday (tm) I could split args into (tool, args) parameters to make it more explicit, and change the way the environment variables are parsed to keep the tool/args separate. Pretty minor kind of thing, though, so I'm not in a hurry. > Reviewed-by: Hanna Reitz > >
Re: [PATCH 04/15] iotests/040: Don't check image pattern on zero-length image
On Tue, Mar 22, 2022, 10:22 AM Hanna Reitz wrote: > On 18.03.22 21:36, John Snow wrote: > > qemu-io fails on read/write with zero-length raw images, so skip these > > when running the zero-length image tests. > > > > Signed-off-by: John Snow > > --- > > tests/qemu-iotests/040 | 14 -- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > Doesn’t look specific to zero-length images, but the fact that we do I/O > beyond the image size, i.e. any image below 1 MB would be affected. > > Anyway, the zero-length image is the only one tested with a size of less > than 1 MB, so this works. > > Reviewed-by: Hanna Reitz > (Check the cover letter, too - this patch is still good, but iotest 040 still fails and I'm not 100% clear as to why.) >
Re: [PATCH] spec: Add NBD_OPT_EXTENDED_HEADERS
On Tue, Dec 07, 2021 at 06:14:23PM +0200, Wouter Verhelst wrote: > On Mon, Dec 06, 2021 at 05:00:47PM -0600, Eric Blake wrote: > > On Mon, Dec 06, 2021 at 02:40:45PM +0300, Vladimir Sementsov-Ogievskiy > > wrote: > > > > Simple reply message > > > > > > > > The simple reply message MUST be sent by the server in response to all > > > > requests if structured replies have not been negotiated using > > > > -`NBD_OPT_STRUCTURED_REPLY`. If structured replies have been > > > > negotiated, a simple > > > > -reply MAY be used as a reply to any request other than `NBD_CMD_READ`, > > > > -but only if the reply has no data payload. The message looks as > > > > -follows: > > > > +`NBD_OPT_STRUCTURED_REPLY`. If structured replies have been > > > > +negotiated, a simple reply MAY be used as a reply to any request other > > > > +than `NBD_CMD_READ`, but only if the reply has no data payload. If > > > > +extended headers were not negotiated using `NBD_OPT_EXTENDED_HEADERS`, > > > > +the message looks as follows: > > > > > > > > S: 32 bits, 0x67446698, magic (`NBD_SIMPLE_REPLY_MAGIC`; used to be > > > > `NBD_REPLY_MAGIC`) > > > > @@ -369,6 +398,16 @@ S: 64 bits, handle > > > > S: (*length* bytes of data if the request is of type `NBD_CMD_READ` > > > > and > > > > *error* is zero) > > > > > > > > +If extended headers were negotiated using `NBD_OPT_EXTENDED_HEADERS`, > > > > +the message looks like: > > > > + > > > > +S: 32 bits, 0x60d12fd6, magic (`NBD_SIMPLE_REPLY_EXT_MAGIC`) > > > > +S: 32 bits, error (MAY be zero) > > > > +S: 64 bits, handle > > > > +S: 128 bits, padding (MUST be zero) > > > > +S: (*length* bytes of data if the request is of type `NBD_CMD_READ` and > > > > +*error* is zero) > > > > + > > > > > > If we go this way, let's put payload length into padding: it will help to > > > make the protocol context-independent and less error-prone. > > Agreed. > > > Easy enough to do (the payload length will be 0 except for > > NBD_CMD_READ). > > Indeed. > > > > Or, the otherway, may be just forbid the payload for simple-64bit ? > > > What's the reason to allow 64bit requests without structured reply > > > negotiation? > > > > The two happened to be orthogonal enough in my implementation. It was > > easy to demonstrate either one without the other, and it IS easier to > > write a client using non-structured replies (structured reads ARE > > tougher than simple reads, even if it is less efficient when it comes > > to reading zeros). But you are also right that we could require > > structured reads prior to allowing 64-bit operations, and then have > > only one supported reply type on the wire when negotiated. Wouter, > > which way do you prefer? > > Given that I *still* haven't gotten around to implementing structured > replies for nbd-server, I'd prefer not to require it, but that's not > really a decent argument IMO :-) > > [... I haven't read this in much detail yet, intend to do that later...] Ping - any other responses on this thread, before I start working on version 2 of the cross-project patches? And repeating a comment from my original cover letter: > with 64-bit commands, we may want to also make it easier to let > servers advertise an actual maximum size they are willing to accept > for the commands in question (for example, a server may be happy with > a full 64-bit block status, but still want to limit non-fast zero and > cache to a smaller limit to avoid denial of service). Is it worth enhancing NBD_OPT_INFO in my v2? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()
On 18.03.22 21:36, John Snow wrote:
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
Signed-off-by: John Snow
---
Note: This breaks several tests at this point. I'll be fixing each
broken test one by one in the subsequent commits. We can squash them all
on merge to avoid test regressions.
Well, absolutely.
(Seems like a way to have your cake and eat it too with regards to
maintaining bisectability while also having nice mailing list patches.)
I personally find reviewability to not be affected whether this is one
patch or multiple, given that the changes are in different files anyway.
I am afraid someone might forgot to squash when merging this series,
though...
Also, I don’t know how to squash R-b tags, and I don’t feel like I can
give an R-b for a patch that decidedly breaks tests.
Copy-pastables:
./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 ones that fail:
./check -qcow2 030 040 242 245
./check -raw migration-permissions
./check -nbd 205
./check -luks 149
Signed-off-by: John Snow
---
tests/qemu-iotests/iotests.py | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 974a2b0c8d..58ea766568 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -354,16 +354,23 @@ def qemu_io_wrap_args(args: Sequence[str]) -> List[str]:
def qemu_io_popen(*args):
return qemu_tool_popen(qemu_io_wrap_args(args))
-def qemu_io(*args):
-'''Run qemu-io and return the stdout data'''
-return qemu_tool_pipe_and_status('qemu-io', qemu_io_wrap_args(args))[0]
+def qemu_io(*args: str, check: bool = True, combine_stdio: bool = True
+) -> subprocess.CompletedProcess[str]:
I guess this return type probably has to be quoted.
+"""
+Run QEMU_IO_PROG and return the status code and console output.
+
+This function always prepends either QEMU_IO_OPTIONS or
+QEMU_IO_OPTIONS_NO_FMT.
+"""
+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):
-result = qemu_io(*args)
-log(result, filters=[filter_testfiles, filter_qemu_io])
+def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:
...and this one.
Hanna
+result = qemu_io(*args, check=False)
+log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
return result
def qemu_io_silent(*args):
Re: Proposal for a regular upstream performance testing
On Mon, Mar 21, 2022 at 11:29:42AM +0100, Lukáš Doktor wrote: > Hello Stefan, > > Dne 21. 03. 22 v 10:42 Stefan Hajnoczi napsal(a): > > On Mon, Mar 21, 2022 at 09:46:12AM +0100, Lukáš Doktor wrote: > >> Dear qemu developers, > >> > >> you might remember the "replied to" email from a bit over year ago to > >> raise a discussion about a qemu performance regression CI. On KVM forum I > >> presented > >> https://www.youtube.com/watch?v=Cbm3o4ACE3Y&list=PLbzoR-pLrL6q4ZzA4VRpy42Ua4-D2xHUR&index=9 > >> some details about my testing pipeline. I think it's stable enough to > >> become part of the official CI so people can consume, rely on it and > >> hopefully even suggest configuration changes. > >> > >> The CI consists of: > >> > >> 1. Jenkins pipeline(s) - internal, not available to developers, running > >> daily builds of the latest available commit > >> 2. Publicly available anonymized results: > >> https://ldoktor.github.io/tmp/RedHat-Perf-worker1/ > > > > This link is 404. > > > > My mistake, it works well without the tailing slash: > https://ldoktor.github.io/tmp/RedHat-Perf-worker1 > > >> 3. (optional) a manual gitlab pulling job which triggered by the Jenkins > >> pipeline when that particular commit is checked > >> > >> The (1) is described here: > >> https://run-perf.readthedocs.io/en/latest/jenkins.html and can be > >> replicated on other premises and the individual jobs can be executed > >> directly https://run-perf.readthedocs.io on any linux box using Fedora > >> guests (via pip or container > >> https://run-perf.readthedocs.io/en/latest/container.html ). > >> > >> As for the (3) I made a testing pipeline available here: > >> https://gitlab.com/ldoktor/qemu/-/pipelines with one always-passing test > >> and one allow-to-fail actual testing job. If you think such integration > >> would be useful, I can add it as another job to the official qemu repo. > >> Note the integration is a bit hacky as, due to resources, we can not test > >> all commits but rather test on daily basis, which is not officially > >> supported by gitlab. > >> > >> Note the aim of this project is to ensure some very basic system-level > >> workflow performance stays the same or that the differences are described > >> and ideally pinned to individual commits. It should not replace thorough > >> release testing or low-level performance tests. > > > > If I understand correctly the GitLab CI integration you described > > follows the "push" model where Jenkins (running on your own machine) > > triggers a manual job in GitLab CI simply to indicate the status of the > > nightly performance regression test? > > > > What process should QEMU follow to handle performance regressions > > identified by your job? In other words, which stakeholders need to > > triage, notify, debug, etc when a regression is identified? > > > > My guess is: > > - Someone (you or the qemu.git committer) need to watch the job status and > > triage failures. > > - That person then notifies likely authors of suspected commits so they can > > investigate. > > - The authors need a way to reproduce the issue - either locally or by > > pushing commits to GitLab and waiting for test results. > > - Fixes will be merged as additional qemu.git commits since commit history > > cannot be rewritten. > > - If necessary a git-revert(1) commit can be merged to temporarily undo a > > commit that caused issues. > > > > Who will watch the job status and triage failures? > > > > Stefan > > This is exactly the main question I'd like to resolve as part of > considering-this-to-be-official-part-of-the-upstream-qemu-testing. At this > point our team is offering it's service to maintain this single worker for > daily jobs, monitoring the status and pinging people in case of bisectable > results. That's great! The main hurdle is finding someone to triage regressions and if you are volunteering to do that then these regression tests would be helpful to QEMU. > From the upstream qemu community we are mainly looking for a feedback: > > * whether they'd want to be notified of such issues (and via what means) I have CCed Kevin Wolf in case he has any questions regarding how fio regressions will be handled. I'm happy to be contacted when a regression bisects to a commit I authored. > * whether the current approach seems to be actually performing useful tasks > * whether the reports are understandable Reports aren't something I would look at as a developer. Although the history and current status may be useful to some maintainers, that information isn't critical. Developers simply need to know which commit introduced a regression and the details of how to run the regression. > * whether the reports should be regularly pushed into publicly available > place (or just on regression/improvement) > * whether there are any volunteers to be interested in non-clearly-bisectable > issues (probably by-topic) One option is to notify maintainers, but when
Re: [PATCH 05/15] iotests: create generic qemu_tool() function
On 18.03.22 21:36, John Snow wrote: reimplement qemu_img() in terms of qemu_tool() in preparation for doing the same with qemu_io(). Signed-off-by: John Snow --- tests/qemu-iotests/iotests.py | 37 +++ 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 6cd8374c81..974a2b0c8d 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 + +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: command-line arguments to a QEMU cli tool. This makes me ask how I am to specify which tool to use. Perhaps it should just be “full command line to run” or something. Might be nice™, but: Reviewed-by: Hanna Reitz
Re: [PATCH 04/15] iotests/040: Don't check image pattern on zero-length image
On 18.03.22 21:36, John Snow wrote: qemu-io fails on read/write with zero-length raw images, so skip these when running the zero-length image tests. Signed-off-by: John Snow --- tests/qemu-iotests/040 | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) Doesn’t look specific to zero-length images, but the fact that we do I/O beyond the image size, i.e. any image below 1 MB would be affected. Anyway, the zero-length image is the only one tested with a size of less than 1 MB, so this works. Reviewed-by: Hanna Reitz
Re: [PATCH 03/15] iotests: Don't check qemu_io() output for specific error strings
On 18.03.22 21:36, John Snow wrote: 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 --- tests/qemu-iotests/040 | 33 - tests/qemu-iotests/056 | 2 +- 2 files changed, 17 insertions(+), 18 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH 02/15] iotests/163: Fix broken qemu-io invocation
On 18.03.22 21:36, John Snow wrote:
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
---
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 e4cd4b230f..c94ad16f4a 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)
I’m actually puzzled by the original intent here. check_img doesn’t
contain 0x00 in that area...
Well.
Reviewed-by: Hanna Reitz
Re: [PATCH 01/15] iotests: replace calls to log(qemu_io(...)) with qemu_io_log()
On 18.03.22 21:36, John Snow wrote:
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
---
tests/qemu-iotests/242 | 2 +-
tests/qemu-iotests/255 | 4 +---
tests/qemu-iotests/303 | 4 ++--
3 files changed, 4 insertions(+), 6 deletions(-)
Perfectly reasonable, but...
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index b3afd36d72..4b7ec16af6 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -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)
This breaks 242 because qemu_io_log is not imported.
Hanna
Re: [PATCH v2 0/3] Use g_new() & friends where that makes obvious sense
On Tue, Mar 15, 2022 at 03:41:53PM +0100, Markus Armbruster wrote: > g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, > for two reasons. One, it catches multiplication overflowing size_t. > Two, it returns T * rather than void *, which lets the compiler catch > more type errors. > > This series only touches allocations with size arguments of the form > sizeof(T). It's mechanical, except for a tiny fix in PATCH 2. > > PATCH 1 adds the Coccinelle script. > > PATCH 2 cleans up the virtio-9p subsystem, and fixes a harmless typing > error uncovered by the cleanup. > > PATCH 3 cleans up everything else. I started to split it up, but > splitting is a lot of decisions, and I just can't see the value. series: Acked-by: Michael S. Tsirkin > For instance, MAINTAINERS tells me to split for subsystem "virtio", > patching > > hw/char/virtio-serial-bus.c > hw/display/virtio-gpu.c > hw/net/virtio-net.c > hw/virtio/virtio-crypto.c > hw/virtio/virtio-iommu.c > hw/virtio/virtio.c > > But it also tells me to split for subsystem "Character devices", > patching > > hw/char/parallel.c | 2 +- > hw/char/riscv_htif.c | 2 +- > hw/char/virtio-serial-bus.c | 6 +- > > and for subsystem "Network devices", patching > > hw/net/virtio-net.c > > and for subsystem "virtio-gpu", patching > > hw/display/virtio-gpu.c > > I guess I'd go with "virtio". Six files down, 103 to go. Thanks, but > no thanks. > > Since the transformation is local to a function call, dropping is > completely safe. We can deal with conflicts by dropping conflicting > hunks, with "git-pull -s recursive -X ours". Or drop entire files > with conflicts. > > If you want me to split off certain parts, please tell me exactly what > you want split off, and I'll gladly do the splitting. I don't mind > the splitting part, I do mind the *thinking* part. > > I backed out two changes made by the Coccinelle script: > scripts/coverity-scan/model.c, because that's special, and > semihosting/config.c, because it has a typing error similar to the one > fixed in PATCH 2, and Alex already posted a patch for it. > > v2: > * PATCH 3: Change to scripts/coverity-scan/model.c dropped [Eric] > * PATCH 3: Change to semihosting/config.c dropped [Alex] > * Commit messages tweaked > > Markus Armbruster (3): > scripts/coccinelle: New use-g_new-etc.cocci > 9pfs: Use g_new() & friends where that makes obvious sense > Use g_new() & friends where that makes obvious sense > > scripts/coccinelle/use-g_new-etc.cocci | 75 > include/qemu/timer.h | 2 +- > accel/kvm/kvm-all.c | 6 +- > accel/tcg/tcg-accel-ops-mttcg.c | 2 +- > accel/tcg/tcg-accel-ops-rr.c | 4 +- > audio/audio.c| 4 +- > audio/audio_legacy.c | 6 +- > audio/dsoundaudio.c | 2 +- > audio/jackaudio.c| 6 +- > audio/paaudio.c | 4 +- > backends/cryptodev.c | 2 +- > contrib/vhost-user-gpu/vhost-user-gpu.c | 2 +- > cpus-common.c| 4 +- > dump/dump.c | 2 +- > hw/9pfs/9p-proxy.c | 2 +- > hw/9pfs/9p-synth.c | 4 +- > hw/9pfs/9p.c | 8 +-- > hw/9pfs/codir.c | 6 +- > hw/acpi/hmat.c | 2 +- > hw/audio/intel-hda.c | 2 +- > hw/char/parallel.c | 2 +- > hw/char/riscv_htif.c | 2 +- > hw/char/virtio-serial-bus.c | 6 +- > hw/core/irq.c| 2 +- > hw/core/reset.c | 2 +- > hw/display/pxa2xx_lcd.c | 2 +- > hw/display/tc6393xb.c| 2 +- > hw/display/virtio-gpu.c | 4 +- > hw/display/xenfb.c | 4 +- > hw/dma/rc4030.c | 4 +- > hw/i2c/core.c| 4 +- > hw/i2c/i2c_mux_pca954x.c | 2 +- > hw/i386/amd_iommu.c | 4 +- > hw/i386/intel_iommu.c| 2 +- > hw/i386/xen/xen-hvm.c| 10 ++-- > hw/i386/xen/xen-mapcache.c | 14 ++--- > hw/input/lasips2.c | 2 +- > hw/input/pckbd.c | 2 +- > hw/input/ps2.c | 4 +- > hw/input/pxa2xx_keypad.c | 2 +- > hw/input/tsc2005.c | 3 +- > hw/intc/riscv_aclint.c | 6 +- > hw/intc/xics.c | 2 +- > hw/m68k/virt.c | 2 +- > hw/mips/mipssim.c| 2 +- > hw/misc/applesmc.c | 2 +- >
[PULL 24/25] iotests.py: Filters for VM.run_job()
Allow filters for VM.run_job(), and pass the filters given to
VM.blockdev_create() to it.
(Use this opportunity to annotate VM.run_job()'s parameter types;
unfortunately, for the filter, I could not come up with anything better
than Callable[[Any], Any] that would pass mypy's scrutiny.)
At one point, a plain string is logged, so the filters passed to it must
work fine with plain strings. The only filters passed to it at this
point are the ones from VM.blockdev_create(), which are
filter_qmp_test_files() (by default) and 207's filter_hash(). Both
cannot handle plain strings yet, but we can make them by amending
filter_qmp() to treat them as plain values with a None key.
Signed-off-by: Hanna Reitz
Message-Id: <20220318125304.66131-2-hre...@redhat.com>
Reviewed-by: John Snow
---
tests/qemu-iotests/iotests.py | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4b788c7491..fcec3e51e5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -607,8 +607,10 @@ def filter_qmp(qmsg, filter_fn):
# Iterate through either lists or dicts;
if isinstance(qmsg, list):
items = enumerate(qmsg)
-else:
+elif isinstance(qmsg, dict):
items = qmsg.items()
+else:
+return filter_fn(None, qmsg)
for k, v in items:
if isinstance(v, (dict, list)):
@@ -944,8 +946,12 @@ def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
return result
# Returns None on success, and an error string on failure
-def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-pre_finalize=None, cancel=False, wait=60.0):
+def run_job(self, job: str, auto_finalize: bool = True,
+auto_dismiss: bool = False,
+pre_finalize: Optional[Callable[[], None]] = None,
+cancel: bool = False, wait: float = 60.0,
+filters: Iterable[Callable[[Any], Any]] = (),
+) -> Optional[str]:
"""
run_job moves a job from creation through to dismissal.
@@ -975,7 +981,7 @@ def run_job(self, job, auto_finalize=True,
auto_dismiss=False,
while True:
ev = filter_qmp_event(self.events_wait(events, timeout=wait))
if ev['event'] != 'JOB_STATUS_CHANGE':
-log(ev)
+log(ev, filters=filters)
continue
status = ev['data']['status']
if status == 'aborting':
@@ -983,18 +989,18 @@ def run_job(self, job, auto_finalize=True,
auto_dismiss=False,
for j in result['return']:
if j['id'] == job:
error = j['error']
-log('Job failed: %s' % (j['error']))
+log('Job failed: %s' % (j['error']), filters=filters)
elif status == 'ready':
-self.qmp_log('job-complete', id=job)
+self.qmp_log('job-complete', id=job, filters=filters)
elif status == 'pending' and not auto_finalize:
if pre_finalize:
pre_finalize()
if cancel:
-self.qmp_log('job-cancel', id=job)
+self.qmp_log('job-cancel', id=job, filters=filters)
else:
-self.qmp_log('job-finalize', id=job)
+self.qmp_log('job-finalize', id=job, filters=filters)
elif status == 'concluded' and not auto_dismiss:
-self.qmp_log('job-dismiss', id=job)
+self.qmp_log('job-dismiss', id=job, filters=filters)
elif status == 'null':
return error
@@ -1007,7 +1013,7 @@ def blockdev_create(self, options, job_id='job0',
filters=None):
if 'return' in result:
assert result['return'] == {}
-job_result = self.run_job(job_id)
+job_result = self.run_job(job_id, filters=filters)
else:
job_result = result['error']
--
2.35.1
[PULL 23/25] iotests: make qemu_img_log and img_info_log raise on error
From: John Snow
Add a `check: bool = True` parameter to both functions and make their
qemu_img() invocations raise on error by default.
users of img_info_log:
206, 207, 210, 211, 212, 213, 237, 242, 266, 274, 302
users of qemu_img_log:
044, 209, 274, 302, 304
iotests 242 and 266 need to use check=False for their negative tests.
iotests 206, 210, 211, 212, 213, 237, 274 and 302 continue working
normally.
As of this commit, all calls to QEMU_IMG made from iotests enforce a
return code of zero by default unless explicitly disabled or suppressed
by passing check=False or with an exception handler.
Signed-off-by: John Snow
Message-Id: <20220321201618.903471-19-js...@redhat.com>
Reviewed-by: Eric Blake
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/242| 2 +-
tests/qemu-iotests/266| 2 +-
tests/qemu-iotests/iotests.py | 8 +---
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 547bf382e3..b3afd36d72 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -100,7 +100,7 @@ add_bitmap(1, True, False)
log('Write an unknown bitmap flag \'{}\' into a new QCOW2 image at offset {}'
.format(hex(bitmap_flag_unknown), flag_offset))
toggle_flag(flag_offset)
-img_info_log(disk)
+img_info_log(disk, check=False)
toggle_flag(flag_offset)
log('Unset the unknown bitmap flag \'{}\' in the bitmap directory entry:\n'
.format(hex(bitmap_flag_unknown)))
diff --git a/tests/qemu-iotests/266 b/tests/qemu-iotests/266
index 71ce81d0df..8fc3807ac5 100755
--- a/tests/qemu-iotests/266
+++ b/tests/qemu-iotests/266
@@ -137,7 +137,7 @@ def main():
iotests.log('')
vm.shutdown()
-iotests.img_info_log(file_path)
+iotests.img_info_log(file_path, check=False)
iotests.script_main(main,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1771d01977..4b788c7491 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -312,13 +312,15 @@ def qemu_img_info(*args: str) -> Any:
def qemu_img_map(*args: str) -> Any:
return qemu_img_json('map', "--output", "json", *args)
-def qemu_img_log(*args: str) -> 'subprocess.CompletedProcess[str]':
-result = qemu_img(*args, check=False)
+def qemu_img_log(*args: str, check: bool = True
+ ) -> 'subprocess.CompletedProcess[str]':
+result = qemu_img(*args, check=check)
log(result.stdout, filters=[filter_testfiles])
return result
def img_info_log(filename: str, filter_path: Optional[str] = None,
use_image_opts: bool = False, extra_args: Sequence[str] = (),
+ check: bool = True,
) -> None:
args = ['info']
if use_image_opts:
@@ -328,7 +330,7 @@ def img_info_log(filename: str, filter_path: Optional[str]
= None,
args += extra_args
args.append(filename)
-output = qemu_img(*args, check=False).stdout
+output = qemu_img(*args, check=check).stdout
if not filter_path:
filter_path = filename
log(filter_img_info(output, filter_path))
--
2.35.1
[PULL 20/25] iotests: use qemu_img() in has_working_luks()
From: John Snow
Admittedly a mostly lateral move, but qemu_img() is essentially the
replacement for qemu_img_pipe_and_status(). It will give slightly better
diagnostics on crash.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-16-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index aaf4da8be4..d006f56127 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1445,20 +1445,20 @@ def has_working_luks() -> Tuple[bool, str]:
"""
img_file = f'{test_dir}/luks-test.luks'
-(output, status) = \
-qemu_img_pipe_and_status('create', '-f', 'luks',
- '--object', luks_default_secret_object,
- '-o', luks_default_key_secret_opt,
- '-o', 'iter-time=10',
- img_file, '1G')
+res = qemu_img('create', '-f', 'luks',
+ '--object', luks_default_secret_object,
+ '-o', luks_default_key_secret_opt,
+ '-o', 'iter-time=10',
+ img_file, '1G',
+ check=False)
try:
os.remove(img_file)
except OSError:
pass
-if status != 0:
-reason = output
-for line in output.splitlines():
+if res.returncode:
+reason = res.stdout
+for line in res.stdout.splitlines():
if img_file + ':' in line:
reason = line.split(img_file + ':', 1)[1].strip()
break
--
2.35.1
[PULL 14/25] iotests/remove-bitmap-from-backing: use qemu_img_info()
From: John Snow
This removes two more usages of qemu_img_pipe() and replaces them with
calls to qemu_img(), which provides better diagnostic information on
failure.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-10-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/tests/remove-bitmap-from-backing | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing
b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index fee3141340..15be32dcb9 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -19,7 +19,7 @@
#
import iotests
-from iotests import log, qemu_img_create, qemu_img, qemu_img_pipe
+from iotests import log, qemu_img_create, qemu_img, qemu_img_info
iotests.script_initialize(supported_fmts=['qcow2'],
unsupported_imgopts=['compat'])
@@ -33,7 +33,7 @@ qemu_img_create('-f', iotests.imgfmt, '-b', base,
qemu_img('bitmap', '--add', base, 'bitmap0')
# Just assert that our method of checking bitmaps in the image works.
-assert 'bitmaps' in qemu_img_pipe('info', base)
+assert 'bitmaps' in qemu_img_info(base)['format-specific']['data']
vm = iotests.VM().add_drive(top, 'backing.node-name=base')
vm.launch()
@@ -68,5 +68,5 @@ if result != {'return': {}}:
vm.shutdown()
-if 'bitmaps' in qemu_img_pipe('info', base):
+if 'bitmaps' in qemu_img_info(base)['format-specific']['data']:
log('ERROR: Bitmap is still in the base image')
--
2.35.1
[PULL 25/25] iotests/207: Filter host fingerprint
Commit e3296cc796aeaf319f3ed4e064ec309baf5e4da4 made the ssh block
driver's error message for fingerprint mismatches more verbose, so it
now prints the actual host key fingerprint and the key type.
iotest 207 tests such errors, but was not amended to filter that
fingerprint (which is host-specific), so do it now. Filter the key
type, too, because I guess this too can differ depending on the host
configuration.
Fixes: e3296cc796aeaf319f3ed4e064ec309baf5e4da4
("block: print the server key type and fingerprint on failure")
Reported-by: John Snow
Signed-off-by: Hanna Reitz
Message-Id: <20220318125304.66131-3-hre...@redhat.com>
Reviewed-by: John Snow
---
tests/qemu-iotests/207 | 7 ++-
tests/qemu-iotests/207.out | 6 +++---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 0f5c4bc8a0..41dcf3ff55 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -35,7 +35,12 @@ def filter_hash(qmsg):
if key == 'hash' and re.match('[0-9a-f]+', value):
return 'HASH'
return value
-return iotests.filter_qmp(qmsg, _filter)
+if isinstance(qmsg, str):
+# Strip key type and fingerprint
+p = r"\S+ (key fingerprint) '(md5|sha1|sha256):[0-9a-f]+'"
+return re.sub(p, r"\1 '\2:HASH'", qmsg)
+else:
+return iotests.filter_qmp(qmsg, _filter)
def blockdev_create(vm, options):
vm.blockdev_create(options, filters=[iotests.filter_qmp_testfiles,
filter_hash])
diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
index aeb8569d77..05cf753283 100644
--- a/tests/qemu-iotests/207.out
+++ b/tests/qemu-iotests/207.out
@@ -42,7 +42,7 @@ virtual size: 4 MiB (4194304 bytes)
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode":
"hash", "type": "md5"}, "path": "TEST_DIR/PID-t.img", "server": {"host":
"127.0.0.1", "port": "22"}}, "size": 2097152}}}
{"return": {}}
-Job failed: remote host key does not match host_key_check 'wrong'
+Job failed: remote host key fingerprint 'md5:HASH' does not match
host_key_check 'md5:wrong'
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -59,7 +59,7 @@ virtual size: 8 MiB (8388608 bytes)
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode":
"hash", "type": "sha1"}, "path": "TEST_DIR/PID-t.img", "server": {"host":
"127.0.0.1", "port": "22"}}, "size": 2097152}}}
{"return": {}}
-Job failed: remote host key does not match host_key_check 'wrong'
+Job failed: remote host key fingerprint 'sha1:HASH' does not match
host_key_check 'sha1:wrong'
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
@@ -76,7 +76,7 @@ virtual size: 4 MiB (4194304 bytes)
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode":
"hash", "type": "sha256"}, "path": "TEST_DIR/PID-t.img", "server": {"host":
"127.0.0.1", "port": "22"}}, "size": 2097152}}}
{"return": {}}
-Job failed: remote host key does not match host_key_check 'wrong'
+Job failed: remote host key fingerprint 'sha256:HASH' does not match
host_key_check 'sha256:wrong'
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
--
2.35.1
[PULL 15/25] iotests: add qemu_img_map() function
From: John Snow
Add a qemu_img_map() function by analogy with qemu_img_measure(),
qemu_img_check(), and qemu_img_info() that all return JSON information.
Replace calls to qemu_img_pipe('map', '--output=json', ...) with this
new function, which provides better diagnostic information on failure.
Note: The output for iotest 211 changes, because logging JSON after it
was deserialized by Python behaves a little differently than logging the
raw JSON document string itself.
(iotests.log() sorts the keys for Python 3.6 support.)
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Message-Id: <20220321201618.903471-11-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/041 | 5 ++---
tests/qemu-iotests/211 | 6 +++---
tests/qemu-iotests/211.out | 10 +++---
tests/qemu-iotests/iotests.py | 3 +++
tests/qemu-iotests/tests/block-status-cache| 11 ---
tests/qemu-iotests/tests/parallels-read-bitmap | 6 ++
6 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index db9f5dc540..3e16acee56 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -24,7 +24,7 @@ import os
import re
import json
import iotests
-from iotests import qemu_img, qemu_img_pipe, qemu_io
+from iotests import qemu_img, qemu_img_map, qemu_io
backing_img = os.path.join(iotests.test_dir, 'backing.img')
target_backing_img = os.path.join(iotests.test_dir, 'target-backing.img')
@@ -1360,8 +1360,7 @@ class TestFilters(iotests.QMPTestCase):
self.vm.qmp('blockdev-del', node_name='target')
-target_map = qemu_img_pipe('map', '--output=json', target_img)
-target_map = json.loads(target_map)
+target_map = qemu_img_map(target_img)
assert target_map[0]['start'] == 0
assert target_map[0]['length'] == 512 * 1024
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index f52cadade1..1a3b4596c8 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -59,7 +59,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
vm.shutdown()
iotests.img_info_log(disk_path)
-iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+iotests.log(iotests.qemu_img_map(disk_path))
#
# Successful image creation (explicit defaults)
@@ -83,7 +83,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
vm.shutdown()
iotests.img_info_log(disk_path)
-iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+iotests.log(iotests.qemu_img_map(disk_path))
#
# Successful image creation (with non-default options)
@@ -107,7 +107,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
vm.shutdown()
iotests.img_info_log(disk_path)
-iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+iotests.log(iotests.qemu_img_map(disk_path))
#
# Invalid BlockdevRef
diff --git a/tests/qemu-iotests/211.out b/tests/qemu-iotests/211.out
index c4425b5982..f02c75409c 100644
--- a/tests/qemu-iotests/211.out
+++ b/tests/qemu-iotests/211.out
@@ -17,8 +17,7 @@ file format: IMGFMT
virtual size: 128 MiB (134217728 bytes)
cluster_size: 1048576
-[{ "start": 0, "length": 134217728, "depth": 0, "present": true, "zero": true,
"data": false}]
-
+[{"data": false, "depth": 0, "length": 134217728, "present": true, "start": 0,
"zero": true}]
=== Successful image creation (explicit defaults) ===
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "file", "filename": "TEST_DIR/PID-t.vdi", "size": 0}}}
@@ -36,8 +35,7 @@ file format: IMGFMT
virtual size: 64 MiB (67108864 bytes)
cluster_size: 1048576
-[{ "start": 0, "length": 67108864, "depth": 0, "present": true, "zero": true,
"data": false}]
-
+[{"data": false, "depth": 0, "length": 67108864, "present": true, "start": 0,
"zero": true}]
=== Successful image creation (with non-default options) ===
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "file", "filename": "TEST_DIR/PID-t.vdi", "size": 0}}}
@@ -55,9 +53,7 @@ file format: IMGFMT
virtual size: 32 MiB (33554432 bytes)
cluster_size: 1048576
-[{ "start": 0, "length": 3072, "depth": 0, "present": true, "zero": false,
"data": true, "offset": 1024},
-{ "start": 3072, "length": 33551360, "depth": 0, "present": true, "zero":
true, "data": true, "offset": 4096}]
-
+[{"data": true, "depth": 0, "length": 3072, "offset": 1024, "present": true,
"start": 0, "zero": false}, {"data": true, "depth": 0, "length": 33551360,
"offset": 4096, "present": true, "start": 3072, "zero": true}]
=== Invalid BlockdevRef ===
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "vdi", "file": "this doesn't exist", "size": 33554432}}}
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2a1c589b76..b3e793
[PULL 22/25] iotests: remove qemu_img_pipe_and_status()
From: John Snow
With the exceptional 'create' calls removed in the prior commit, change
qemu_img_log() and img_info_log() to call qemu_img() directly
instead.
For now, allow these calls to qemu-img to return non-zero on the basis
that any unusual output will be logged anyway. The very next commit
begins to enforce a successful exit code by default even for the logged
functions.
Signed-off-by: John Snow
Message-Id: <20220321201618.903471-18-js...@redhat.com>
Reviewed-by: Eric Blake
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 26 +++---
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d006f56127..1771d01977 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -207,15 +207,6 @@ def qemu_img_create_prepare_args(args: List[str]) ->
List[str]:
return result
-def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
-"""
-Run qemu-img and return both its output and its exit code
-"""
-is_create = bool(args and args[0] == 'create')
-full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
-return qemu_tool_pipe_and_status('qemu-img', full_args,
- drop_successful_output=is_create)
-
def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
) -> 'subprocess.CompletedProcess[str]':
"""
@@ -321,17 +312,14 @@ def qemu_img_info(*args: str) -> Any:
def qemu_img_map(*args: str) -> Any:
return qemu_img_json('map', "--output", "json", *args)
-def qemu_img_pipe(*args: str) -> str:
-'''Run qemu-img and return its output'''
-return qemu_img_pipe_and_status(*args)[0]
-
-def qemu_img_log(*args):
-result = qemu_img_pipe(*args)
-log(result, filters=[filter_testfiles])
+def qemu_img_log(*args: str) -> 'subprocess.CompletedProcess[str]':
+result = qemu_img(*args, check=False)
+log(result.stdout, filters=[filter_testfiles])
return result
-def img_info_log(filename, filter_path=None, use_image_opts=False,
- extra_args=()):
+def img_info_log(filename: str, filter_path: Optional[str] = None,
+ use_image_opts: bool = False, extra_args: Sequence[str] = (),
+ ) -> None:
args = ['info']
if use_image_opts:
args.append('--image-opts')
@@ -340,7 +328,7 @@ def img_info_log(filename, filter_path=None,
use_image_opts=False,
args += extra_args
args.append(filename)
-output = qemu_img_pipe(*args)
+output = qemu_img(*args, check=False).stdout
if not filter_path:
filter_path = filename
log(filter_img_info(output, filter_path))
--
2.35.1
[PULL 19/25] iotests: remove remaining calls to qemu_img_pipe()
From: John Snow
As part of moving all python iotest invocations of qemu-img onto a
single qemu_img() implementation, remove a few lingering uses of
qemu_img_pipe() from outside of iotests.py itself.
Several cases here rely on the knowledge that qemu_img_pipe() suppresses
*all* output on a successful case when the command being issued is
'create'.
065: This call's output is inspected, but it appears as if it's expected
to succeed. Replace this call with the checked qemu_img() variant
instead to get better diagnostics if/when qemu-img itself fails.
237: "create" call output isn't actually logged. Use qemu_img_create()
instead, which checks the return code. Remove the empty lines from
the test output.
296: Two calls;
-create: Expected to succeed. Like other create calls, the output
isn't actually logged. Switch to a checked variant
(qemu_img_create) instead. The output for this test is
a mixture of both test styles, so actually replace the
blank line for readability.
-amend: This is expected to fail. Log the output.
After this patch, the only uses of qemu_img_pipe are internal to
iotests.py and will be removed in subsequent patches.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-15-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/065 | 4 ++--
tests/qemu-iotests/237 | 3 +--
tests/qemu-iotests/237.out | 3 ---
tests/qemu-iotests/296 | 12 ++--
4 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index 9466ce7df4..ba94e19349 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
import re
import json
import iotests
-from iotests import qemu_img, qemu_img_info, qemu_img_pipe
+from iotests import qemu_img, qemu_img_info
import unittest
test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -54,7 +54,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
self.assertEqual(data['data'], self.json_compare)
def test_human(self):
-data = qemu_img_pipe('info', '--output=human', test_img).split('\n')
+data = qemu_img('info', '--output=human', test_img).stdout.split('\n')
data = data[(data.index('Format specific information:') + 1)
:data.index('')]
for field in data:
diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
index 43dfd3bd40..5ea13eb01f 100755
--- a/tests/qemu-iotests/237
+++ b/tests/qemu-iotests/237
@@ -165,8 +165,7 @@ with iotests.FilePath('t.vmdk') as disk_path, \
iotests.log("")
for path in [ extent1_path, extent2_path, extent3_path ]:
-msg = iotests.qemu_img_pipe('create', '-f', imgfmt, path, '0')
-iotests.log(msg, [iotests.filter_testfiles])
+iotests.qemu_img_create('-f', imgfmt, path, '0')
vm.add_blockdev('driver=file,filename=%s,node-name=ext1' % (extent1_path))
vm.add_blockdev('driver=file,filename=%s,node-name=ext2' % (extent2_path))
diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out
index aeb9724492..62b8865677 100644
--- a/tests/qemu-iotests/237.out
+++ b/tests/qemu-iotests/237.out
@@ -129,9 +129,6 @@ Job failed: Cannot find device='this doesn't exist' nor
node-name='this doesn't
=== Other subformats ===
-
-
-
== Missing extent ==
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
{"driver": "vmdk", "file": "node0", "size": 33554432, "subformat":
"monolithicFlat"}}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index f80ef3434a..0d21b740a7 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -76,7 +76,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
# create the encrypted block device using qemu-img
def createImg(self, file, secret):
-output = iotests.qemu_img_pipe(
+iotests.qemu_img(
'create',
'--object', *secret.to_cmdline_object(),
'-f', iotests.imgfmt,
@@ -84,8 +84,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
'-o', 'iter-time=10',
file,
'1M')
-
-iotests.log(output, filters=[iotests.filter_test_dir])
+iotests.log('')
# attempts to add a key using qemu-img
def addKey(self, file, secret, new_secret):
@@ -99,7 +98,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
}
}
-output = iotests.qemu_img_pipe(
+output = iotests.qemu_img(
'amend',
'--object', *secret.to_cmdline_object(),
'--object', *new_secret.to_cmdline_object(),
@@ -108,8 +107,9 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
'-o', 'new-secret=' + new_secret.id(),
'-o', 'iter-time=10',
-"json:" + json.dumps(image_options)
-)
+"json:" + json.dump
[PULL 21/25] iotests: replace qemu_img_log('create', ...) calls
From: John Snow
qemu_img_log() calls into qemu_img_pipe(), which always removes output
for 'create' commands on success anyway. Replace all of these calls to
the simpler qemu_img_create(...) which doesn't log, but raises a
detailed exception object on failure instead.
Blank lines are removed from output files where appropriate.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-17-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/255 | 8
tests/qemu-iotests/255.out | 4
tests/qemu-iotests/274 | 17 -
tests/qemu-iotests/274.out | 29 -
tests/qemu-iotests/280 | 2 +-
tests/qemu-iotests/280.out | 1 -
6 files changed, 13 insertions(+), 48 deletions(-)
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 3d6d0e80cb..f86fa851b6 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -42,8 +42,8 @@ with iotests.FilePath('t.qcow2') as disk_path, \
size_str = str(size)
iotests.create_image(base_path, size)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, mid_path, size_str)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, disk_path, size_str)
+iotests.qemu_img_create('-f', iotests.imgfmt, mid_path, size_str)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk_path, size_str)
# Create a backing chain like this:
# base <- [throttled: bps-read=4096] <- mid <- overlay
@@ -92,8 +92,8 @@ with iotests.FilePath('src.qcow2') as src_path, \
size = 128 * 1024 * 1024
size_str = str(size)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, src_path, size_str)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, dst_path, size_str)
+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),
diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
index 11a05a5213..2e837cbb5f 100644
--- a/tests/qemu-iotests/255.out
+++ b/tests/qemu-iotests/255.out
@@ -3,8 +3,6 @@ Finishing a commit job with background reads
=== Create backing chain and start VM ===
-
-
=== Start background read requests ===
=== Run a commit job ===
@@ -21,8 +19,6 @@ Closing the VM while a job is being cancelled
=== Create images and start VM ===
-
-
wrote 1048576/1048576 bytes at offset 0
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
index 080a90f10f..2495e051a2 100755
--- a/tests/qemu-iotests/274
+++ b/tests/qemu-iotests/274
@@ -31,12 +31,11 @@ size_long = 2 * 1024 * 1024
size_diff = size_long - size_short
def create_chain() -> None:
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
- str(size_long))
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base,
- '-F', iotests.imgfmt, mid, str(size_short))
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid,
- '-F', iotests.imgfmt, top, str(size_long))
+iotests.qemu_img_create('-f', iotests.imgfmt, base, str(size_long))
+iotests.qemu_img_create('-f', iotests.imgfmt, '-b', base,
+'-F', iotests.imgfmt, mid, str(size_short))
+iotests.qemu_img_create('-f', iotests.imgfmt, '-b', mid,
+'-F', iotests.imgfmt, top, str(size_long))
iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
@@ -160,9 +159,9 @@ with iotests.FilePath('base') as base, \
('off', '512k', '256k', '500k', '436k')]:
iotests.log('=== preallocation=%s ===' % prealloc)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, base_size)
-iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base,
- '-F', iotests.imgfmt, top, top_size_old)
+iotests.qemu_img_create('-f', iotests.imgfmt, base, base_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, '-b', base,
+'-F', iotests.imgfmt, top, top_size_old)
iotests.qemu_io_log('-c', 'write -P 1 %s 64k' % off, base)
# After this, top_size_old to base_size should be allocated/zeroed.
diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
index 1ce40d839a..acd8b166a6 100644
--- a/tests/qemu-iotests/274.out
+++ b/tests/qemu-iotests/274.out
@@ -1,7 +1,4 @@
== Commit tests ==
-
-
-
wrote 2097152/2097152 bytes at offset 0
2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -63,9 +60,6 @@ read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
=== Testing HMP commit (top -> mid) ===
-
-
-
wrote 2097152/2097152 bytes at offset 0
2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/
[PULL 11/25] iotests: add qemu_img_json()
From: John Snow
qemu_img_json() is a new helper built on top of qemu_img() that tries to
pull a valid JSON document out of the stdout stream.
In the event that the return code is negative (the program crashed), or
the code is greater than zero and did not produce valid JSON output, the
VerboseProcessError raised by qemu_img() is re-raised.
In the event that the return code is zero but we can't parse valid JSON,
allow the JSON deserialization error to be raised.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Message-Id: <20220321201618.903471-7-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 32
1 file changed, 32 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9351f9c6ac..56aa068277 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -277,6 +277,38 @@ def ordered_qmp(qmsg, conv_keys=True):
def qemu_img_create(*args: str) -> 'subprocess.CompletedProcess[str]':
return qemu_img('create', *args)
+def qemu_img_json(*args: str) -> Any:
+"""
+Run qemu-img and return its output as deserialized JSON.
+
+:raise CalledProcessError:
+When qemu-img crashes, or returns a non-zero exit code without
+producing a valid JSON document to stdout.
+:raise JSONDecoderError:
+When qemu-img returns 0, but failed to produce a valid JSON document.
+
+:return: A deserialized JSON object; probably a dict[str, Any].
+"""
+try:
+res = qemu_img(*args, combine_stdio=False)
+except subprocess.CalledProcessError as exc:
+# Terminated due to signal. Don't bother.
+if exc.returncode < 0:
+raise
+
+# Commands like 'check' can return failure (exit codes 2 and 3)
+# to indicate command completion, but with errors found. For
+# multi-command flexibility, ignore the exact error codes and
+# *try* to load JSON.
+try:
+return json.loads(exc.stdout)
+except json.JSONDecodeError:
+# Nope. This thing is toast. Raise the /process/ error.
+pass
+raise
+
+return json.loads(res.stdout)
+
def qemu_img_measure(*args):
return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
--
2.35.1
[PULL 09/25] iotests: make qemu_img raise on non-zero rc by default
From: John Snow
re-write qemu_img() as a function that will by default raise a
VerboseProcessException (extended from CalledProcessException) on
non-zero return codes. This will produce a stack trace that will show
the command line arguments and return code from the failed process run.
Users that want something more flexible (there appears to be only one)
can use check=False and manage the return themselves. However, when the
return code is negative, the Exception will be raised no matter what.
This is done under the belief that there's no legitimate reason, even in
negative tests, to see a crash from qemu-img.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-5-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/257| 8 +++--
tests/qemu-iotests/iotests.py | 57 ++-
2 files changed, 55 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index fb5359c581..e7e7a2317e 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -241,11 +241,13 @@ def compare_images(image, reference, baseimg=None,
expected_match=True):
expected_ret = 0 if expected_match else 1
if baseimg:
qemu_img("rebase", "-u", "-b", baseimg, '-F', iotests.imgfmt, image)
-ret = qemu_img("compare", image, reference)
+
+sub = qemu_img("compare", image, reference, check=False)
+
log('qemu_img compare "{:s}" "{:s}" ==> {:s}, {:s}'.format(
image, reference,
-"Identical" if ret == 0 else "Mismatch",
-"OK!" if ret == expected_ret else "ERROR!"),
+"Identical" if sub.returncode == 0 else "Mismatch",
+"OK!" if sub.returncode == expected_ret else "ERROR!"),
filters=[iotests.filter_testfiles])
def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 508adade9e..4afe63df07 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,9 +37,10 @@
from contextlib import contextmanager
+from qemu.aqmp.legacy import QEMUMonitorProtocol
from qemu.machine import qtest
from qemu.qmp import QMPMessage
-from qemu.aqmp.legacy import QEMUMonitorProtocol
+from qemu.utils import VerboseProcessError
# Use this logger for logging messages directly from the iotests module
logger = logging.getLogger('qemu.iotests')
@@ -215,9 +216,50 @@ def qemu_img_pipe_and_status(*args: str) -> Tuple[str,
int]:
return qemu_tool_pipe_and_status('qemu-img', full_args,
drop_successful_output=is_create)
-def qemu_img(*args: str) -> int:
-'''Run qemu-img and return the exit code'''
-return qemu_img_pipe_and_status(*args)[1]
+def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
+ ) -> 'subprocess.CompletedProcess[str]':
+"""
+Run qemu_img and return the 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 check: Enforce a return code of zero.
+:param combine_stdio: set to False to keep stdout/stderr separated.
+
+:raise VerboseProcessError:
+When the return code is negative, or on any non-zero exit code
+when 'check=True' was provided (the default). This exception has
+'stdout', 'stderr', and 'returncode' properties that may be
+inspected to show greater detail. If this exception is not
+handled, the command-line, return code, and all console output
+will be included at the bottom of the stack trace.
+
+:return:
+a CompletedProcess. This object has args, returncode, and stdout
+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,
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT if combine_stdio else subprocess.PIPE,
+universal_newlines=True,
+check=False
+)
+
+if check and subp.returncode or (subp.returncode < 0):
+raise VerboseProcessError(
+subp.returncode, full_args,
+output=subp.stdout,
+stderr=subp.stderr,
+)
+
+return subp
+
def ordered_qmp(qmsg, conv_keys=True):
# Dictionaries are not ordered prior to 3.6, therefore:
@@ -232,7 +274,7 @@ def ordered_qmp(qmsg, conv_keys=True):
return od
return qmsg
-def qemu_img_create(*args):
+def qemu_img_create(*args: str) -> 'subprocess.CompletedProcess[str]':
return qemu_img('create', *args)
def qemu_img_measure(*args):
@@ -467,8 +509,9 @@ def qemu_nbd_popen(*args):
def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
'''Return True if two ima
[PULL 18/25] iotests/149: Remove qemu_img_pipe() call
From: John Snow
qemu_img_pipe calls blank their output when the command being run is a
'create' call and the command succeeds. Thus, the normative output for
this command in iotest 149 is to print a blank line. We can remove the
logging from this invocation and use a checked invocation, but we still
need to inspect the actual output to see if we want to retroactively
skip the test due to missing cipher support.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-14-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/149 | 7 +--
tests/qemu-iotests/149.out | 21 -
2 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index d49646ca60..9bb96d6a1d 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -265,8 +265,11 @@ def qemu_img_create(config, size_mb):
"%dM" % size_mb]
iotests.log("qemu-img " + " ".join(args),
filters=[iotests.filter_test_dir])
-iotests.log(check_cipher_support(config, iotests.qemu_img_pipe(*args)),
-filters=[iotests.filter_test_dir])
+try:
+iotests.qemu_img(*args)
+except subprocess.CalledProcessError as exc:
+check_cipher_support(config, exc.output)
+raise
def qemu_io_image_args(config, dev=False):
"""Get the args for access an image or device with qemu-io"""
diff --git a/tests/qemu-iotests/149.out b/tests/qemu-iotests/149.out
index ab879596ce..2cc5b82f7c 100644
--- a/tests/qemu-iotests/149.out
+++ b/tests/qemu-iotests/149.out
@@ -61,7 +61,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha1.img
# = qemu-img aes-256-xts-plain64-sha1 =
# Create image
qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o
key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1
TEST_DIR/luks-aes-256-xts-plain64-sha1.img 4194304M
-
# Open dev
sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha1.img
qiotest-145-aes-256-xts-plain64-sha1
# Write test pattern 0xa7
@@ -180,7 +179,6 @@ unlink TEST_DIR/luks-twofish-256-xts-plain64-sha1.img
# = qemu-img twofish-256-xts-plain64-sha1 =
# Create image
qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o
key-secret=sec0,iter-time=10,cipher-alg=twofish-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1
TEST_DIR/luks-twofish-256-xts-plain64-sha1.img 4194304M
-
# Open dev
sudo cryptsetup -q -v luksOpen TEST_DIR/luks-twofish-256-xts-plain64-sha1.img
qiotest-145-twofish-256-xts-plain64-sha1
# Write test pattern 0xa7
@@ -299,7 +297,6 @@ unlink TEST_DIR/luks-serpent-256-xts-plain64-sha1.img
# = qemu-img serpent-256-xts-plain64-sha1 =
# Create image
qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o
key-secret=sec0,iter-time=10,cipher-alg=serpent-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1
TEST_DIR/luks-serpent-256-xts-plain64-sha1.img 4194304M
-
# Open dev
sudo cryptsetup -q -v luksOpen TEST_DIR/luks-serpent-256-xts-plain64-sha1.img
qiotest-145-serpent-256-xts-plain64-sha1
# Write test pattern 0xa7
@@ -418,7 +415,6 @@ unlink TEST_DIR/luks-cast5-128-cbc-plain64-sha1.img
# = qemu-img cast5-128-cbc-plain64-sha1 =
# Create image
qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o
key-secret=sec0,iter-time=10,cipher-alg=cast5-128,cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha1
TEST_DIR/luks-cast5-128-cbc-plain64-sha1.img 4194304M
-
# Open dev
sudo cryptsetup -q -v luksOpen TEST_DIR/luks-cast5-128-cbc-plain64-sha1.img
qiotest-145-cast5-128-cbc-plain64-sha1
# Write test pattern 0xa7
@@ -538,7 +534,6 @@ unlink TEST_DIR/luks-aes-256-cbc-plain-sha1.img
# = qemu-img aes-256-cbc-plain-sha1 =
# Create image
qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o
key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=cbc,ivgen-alg=plain,hash-alg=sha1
TEST_DIR/luks-aes-256-cbc-plain-sha1.img 4194304M
-
# Open dev
sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-cbc-plain-sha1.img
qiotest-145-aes-256-cbc-plain-sha1
# Write test pattern 0xa7
@@ -657,7 +652,6 @@ unlink TEST_DIR/luks-aes-256-cbc-plain64-sha1.img
# = qemu-img aes-256-cbc-plain64-sha1 =
# Create image
qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o
key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha1
TEST_DIR/luks-aes-256-cbc-plain64-sha1.img 4194304M
-
# Open dev
sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-cbc-plain64-sha1.img
qiotest-145-aes-256-cbc-plain64-sha1
# Write test pattern 0xa7
@@ -776,7 +770,6 @@ unlink TEST_DIR/luks-aes-256-cbc-essiv-sha256-sha1.img
# ===
[PULL 17/25] iotests: replace unchecked calls to qemu_img_pipe()
From: John Snow
qemu_img_pipe() discards the return code from qemu-img in favor of
returning just its output. Some tests using this function don't save,
log, or check the output either, though, which is unsafe.
Replace all of these calls with a checked version.
Tests affected are 194, 202, 203, 234, 262, and 303.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-13-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/194 | 4 ++--
tests/qemu-iotests/202 | 4 ++--
tests/qemu-iotests/203 | 4 ++--
tests/qemu-iotests/234 | 4 ++--
tests/qemu-iotests/262 | 2 +-
tests/qemu-iotests/303 | 2 +-
6 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index e44b8df728..68894371f5 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -33,8 +33,8 @@ with iotests.FilePath('source.img') as source_img_path, \
iotests.VM('dest') as dest_vm:
img_size = '1G'
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, source_img_path,
img_size)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, dest_img_path,
img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, source_img_path, img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, dest_img_path, img_size)
iotests.log('Launching VMs...')
(source_vm.add_drive(source_img_path)
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 8eb5f32d15..b784dcd791 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -35,8 +35,8 @@ with iotests.FilePath('disk0.img') as disk0_img_path, \
iotests.VM() as vm:
img_size = '10M'
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path,
img_size)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path,
img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk0_img_path, img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk1_img_path, img_size)
iotests.log('Launching VM...')
vm.launch()
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index ea30e50497..ab80fd0e44 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -33,8 +33,8 @@ with iotests.FilePath('disk0.img') as disk0_img_path, \
iotests.VM() as vm:
img_size = '10M'
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path,
img_size)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path,
img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk0_img_path, img_size)
+iotests.qemu_img_create('-f', iotests.imgfmt, disk1_img_path, img_size)
iotests.log('Launching VM...')
(vm.add_object('iothread,id=iothread0')
diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
index cb5f1753e0..a9f764bb2c 100755
--- a/tests/qemu-iotests/234
+++ b/tests/qemu-iotests/234
@@ -34,8 +34,8 @@ with iotests.FilePath('img') as img_path, \
iotests.VM(path_suffix='a') as vm_a, \
iotests.VM(path_suffix='b') as vm_b:
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, backing_path, '64M')
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+iotests.qemu_img_create('-f', iotests.imgfmt, backing_path, '64M')
+iotests.qemu_img_create('-f', iotests.imgfmt, img_path, '64M')
os.mkfifo(fifo_a)
os.mkfifo(fifo_b)
diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index 32d69988ef..2294fd5ecb 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -51,7 +51,7 @@ with iotests.FilePath('img') as img_path, \
vm.add_device('virtio-blk,drive=%s,iothread=iothread0' % root)
-iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+iotests.qemu_img_create('-f', iotests.imgfmt, img_path, '64M')
os.mkfifo(fifo)
diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 16c2e10827..93aa5ce9b7 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -38,7 +38,7 @@ def create_bitmap(bitmap_number, disabled):
if disabled:
args.append('--disable')
-iotests.qemu_img_pipe(*args)
+iotests.qemu_img(*args)
def write_to_disk(offset, size):
--
2.35.1
[PULL 07/25] python/utils: add VerboseProcessError
From: John Snow
This adds an Exception that extends the Python stdlib
subprocess.CalledProcessError.
The difference is that the str() method of this exception also adds the
stdout/stderr logs. In effect, if this exception goes unhandled, Python
will print the output in a visually distinct wrapper to the terminal so
that it's easy to spot in a sea of traceback information.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-3-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
python/qemu/utils/__init__.py | 39 +++
1 file changed, 39 insertions(+)
diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
index b84c86d004..9fb273b13d 100644
--- a/python/qemu/utils/__init__.py
+++ b/python/qemu/utils/__init__.py
@@ -18,6 +18,7 @@
import os
import re
import shutil
+from subprocess import CalledProcessError
import textwrap
from typing import Optional
@@ -26,6 +27,7 @@
__all__ = (
+'VerboseProcessError',
'add_visual_margin',
'get_info_usernet_hostfwd_port',
'kvm_available',
@@ -121,3 +123,40 @@ def _wrap(line: str) -> str:
os.linesep.join(_wrap(line) for line in content.splitlines()),
_bar(None, top=False),
))
+
+
+class VerboseProcessError(CalledProcessError):
+"""
+The same as CalledProcessError, but more verbose.
+
+This is useful for debugging failed calls during test executions.
+The return code, signal (if any), and terminal output will be displayed
+on unhandled exceptions.
+"""
+def summary(self) -> str:
+"""Return the normal CalledProcessError str() output."""
+return super().__str__()
+
+def __str__(self) -> str:
+lmargin = ' '
+width = -len(lmargin)
+sections = []
+
+# Does self.stdout contain both stdout and stderr?
+has_combined_output = self.stderr is None
+
+name = 'output' if has_combined_output else 'stdout'
+if self.stdout:
+sections.append(add_visual_margin(self.stdout, width, name))
+else:
+sections.append(f"{name}: N/A")
+
+if self.stderr:
+sections.append(add_visual_margin(self.stderr, width, 'stderr'))
+elif not has_combined_output:
+sections.append("stderr: N/A")
+
+return os.linesep.join((
+self.summary(),
+textwrap.indent(os.linesep.join(sections), prefix=lmargin),
+))
--
2.35.1
[PULL 10/25] iotests: fortify compare_images() against crashes
From: John Snow
Fortify compare_images() to be more discerning about the status codes it
receives. If qemu_img() returns an exit code that implies it didn't
actually perform the comparison, treat that as an exceptional
circumstance and force the caller to be aware of the peril.
If a negative test is desired (perhaps to test how qemu_img compare
behaves on malformed images, for instance), it is still possible to
catch the exception in the test and deal with that circumstance
manually.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-6-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4afe63df07..9351f9c6ac 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -507,11 +507,22 @@ def qemu_nbd_popen(*args):
p.kill()
p.wait()
-def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
-'''Return True if two image files are identical'''
-res = qemu_img('compare', '-f', fmt1,
- '-F', fmt2, img1, img2, check=False)
-return res.returncode == 0
+def compare_images(img1: str, img2: str,
+ fmt1: str = imgfmt, fmt2: str = imgfmt) -> bool:
+"""
+Compare two images with QEMU_IMG; return True if they are identical.
+
+:raise CalledProcessError:
+when qemu-img crashes or returns a status code of anything other
+than 0 (identical) or 1 (different).
+"""
+try:
+qemu_img('compare', '-f', fmt1, '-F', fmt2, img1, img2)
+return True
+except subprocess.CalledProcessError as exc:
+if exc.returncode == 1:
+return False
+raise
def create_image(name, size):
'''Create a fully-allocated raw image with sector markers'''
--
2.35.1
[PULL 16/25] iotests: change supports_quorum to use qemu_img
From: John Snow
Similar to other recent changes: use the qemu_img() invocation that
supports throwing loud, nasty exceptions when it fails for surprising
reasons.
(Why would "--help" ever fail? I don't know, but eliminating *all* calls
to qemu-img that do not go through qemu_img() is my goal, so
qemu_img_pipe() has to be removed.)
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-12-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b3e793f78a..aaf4da8be4 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1428,8 +1428,8 @@ def _verify_imgopts(unsupported: Sequence[str] = ()) ->
None:
notrun(f'not suitable for this imgopts: {imgopts}')
-def supports_quorum():
-return 'quorum' in qemu_img_pipe('--help')
+def supports_quorum() -> bool:
+return 'quorum' in qemu_img('--help').stdout
def verify_quorum():
'''Skip test suite if quorum support is not available'''
--
2.35.1
[PULL 13/25] iotests: add qemu_img_info()
From: John Snow
Add qemu_img_info() by analogy with qemu_img_measure() and
qemu_img_check(). Modify image_size() to use this function instead to
take advantage of the better diagnostic information on failure provided
(ultimately) by qemu_img().
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-9-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/065| 5 ++---
tests/qemu-iotests/242| 5 ++---
tests/qemu-iotests/iotests.py | 15 +++
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index f7c1b68dad..9466ce7df4 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
import re
import json
import iotests
-from iotests import qemu_img, qemu_img_pipe
+from iotests import qemu_img, qemu_img_info, qemu_img_pipe
import unittest
test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -49,8 +49,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
human_compare = None
def test_json(self):
-data = json.loads(qemu_img_pipe('info', '--output=json', test_img))
-data = data['format-specific']
+data = qemu_img_info(test_img)['format-specific']
self.assertEqual(data['type'], iotests.imgfmt)
self.assertEqual(data['data'], self.json_compare)
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 96a30152b0..547bf382e3 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -22,7 +22,7 @@
import iotests
import json
import struct
-from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
+from iotests import qemu_img_create, qemu_io, qemu_img_info, \
file_path, img_info_log, log, filter_qemu_io
iotests.script_initialize(supported_fmts=['qcow2'],
@@ -39,8 +39,7 @@ flag_offset = 0x5000f
def print_bitmap(extra_args):
log('qemu-img info dump:\n')
img_info_log(disk, extra_args=extra_args)
-result = json.loads(qemu_img_pipe('info', '--force-share',
- '--output=json', disk))
+result = qemu_img_info('--force-share', disk)
if 'bitmaps' in result['format-specific']['data']:
bitmaps = result['format-specific']['data']['bitmaps']
log('The same bitmaps in JSON format:')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4725313150..2a1c589b76 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -315,6 +315,9 @@ def qemu_img_measure(*args: str) -> Any:
def qemu_img_check(*args: str) -> Any:
return qemu_img_json("check", "--output", "json", *args)
+def qemu_img_info(*args: str) -> Any:
+return qemu_img_json('info', "--output", "json", *args)
+
def qemu_img_pipe(*args: str) -> str:
'''Run qemu-img and return its output'''
return qemu_img_pipe_and_status(*args)[0]
@@ -565,10 +568,14 @@ def create_image(name, size):
file.write(sector)
i = i + 512
-def image_size(img):
-'''Return image's virtual size'''
-r = qemu_img_pipe('info', '--output=json', '-f', imgfmt, img)
-return json.loads(r)['virtual-size']
+def image_size(img: str) -> int:
+"""Return image's virtual size"""
+value = qemu_img_info('-f', imgfmt, img)['virtual-size']
+if not isinstance(value, int):
+type_name = type(value).__name__
+raise TypeError("Expected 'int' for 'virtual-size', "
+f"got '{value}' of type '{type_name}'")
+return value
def is_str(val):
return isinstance(val, str)
--
2.35.1
[PULL 08/25] iotests: Remove explicit checks for qemu_img() == 0
From: John Snow
qemu_img() returning zero ought to be the rule, not the
exception. Remove all explicit checks against the condition in
preparation for making non-zero returns an Exception.
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-4-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/163 | 9 +++--
tests/qemu-iotests/216 | 6 +++---
tests/qemu-iotests/218 | 2 +-
tests/qemu-iotests/224 | 11 +--
tests/qemu-iotests/228 | 12 ++--
tests/qemu-iotests/257 | 3 +--
tests/qemu-iotests/258 | 4 ++--
tests/qemu-iotests/310 | 13 ++---
tests/qemu-iotests/tests/block-status-cache | 3 +--
tests/qemu-iotests/tests/graph-changes-while-io | 7 +++
tests/qemu-iotests/tests/image-fleecing | 10 +-
tests/qemu-iotests/tests/mirror-ready-cancel-error | 6 ++
tests/qemu-iotests/tests/mirror-top-perms | 3 +--
tests/qemu-iotests/tests/remove-bitmap-from-backing | 8
tests/qemu-iotests/tests/stream-error-on-reset | 4 ++--
15 files changed, 45 insertions(+), 56 deletions(-)
diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
index b8bfc95358..e4cd4b230f 100755
--- a/tests/qemu-iotests/163
+++ b/tests/qemu-iotests/163
@@ -107,8 +107,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
if iotests.imgfmt == 'raw':
return
-self.assertEqual(qemu_img('check', test_img), 0,
- "Verifying image corruption")
+qemu_img('check', test_img)
def test_empty_image(self):
qemu_img('resize', '-f', iotests.imgfmt, '--shrink', test_img,
@@ -130,8 +129,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
qemu_img('resize', '-f', iotests.imgfmt, '--shrink', test_img,
self.shrink_size)
-self.assertEqual(qemu_img("compare", test_img, check_img), 0,
- "Verifying image content")
+qemu_img("compare", test_img, check_img)
self.image_verify()
@@ -146,8 +144,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
qemu_img('resize', '-f', iotests.imgfmt, '--shrink', test_img,
self.shrink_size)
-self.assertEqual(qemu_img("compare", test_img, check_img), 0,
- "Verifying image content")
+qemu_img("compare", test_img, check_img)
self.image_verify()
diff --git a/tests/qemu-iotests/216 b/tests/qemu-iotests/216
index c02f8d2880..88b385afa3 100755
--- a/tests/qemu-iotests/216
+++ b/tests/qemu-iotests/216
@@ -51,10 +51,10 @@ with iotests.FilePath('base.img') as base_img_path, \
log('--- Setting up images ---')
log('')
-assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
-assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
-'-F', iotests.imgfmt, top_img_path) == 0
+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
log('Done')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 4922b4d3b6..853ed52b34 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -145,7 +145,7 @@ log('')
with iotests.VM() as vm, \
iotests.FilePath('src.img') as src_img_path:
-assert qemu_img('create', '-f', iotests.imgfmt, src_img_path, '64M') == 0
+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
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
index 38dd153625..c31c55b49d 100755
--- a/tests/qemu-iotests/224
+++ b/tests/qemu-iotests/224
@@ -47,12 +47,11 @@ for filter_node_name in False, True:
iotests.FilePath('top.img') as top_img_path, \
iotests.VM() as vm:
-assert qemu_img('create', '-f', iotests.imgfmt,
-base_img_path, '64M') == 0
-assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
-'-F', iotests.imgfmt, mid_img_path) == 0
-assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
-'-F', iotests.imgfmt, top_img_path) == 0
+qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
+qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+ '-F', iotests.imgfmt, mid_img_path)
+qemu_img('create
[PULL 12/25] iotests: use qemu_img_json() when applicable
From: John Snow
qemu_img_json() gives better diagnostic information on failure.
Signed-off-by: John Snow
Reviewed-by: Hanna Reitz
Message-Id: <20220321201618.903471-8-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 56aa068277..4725313150 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -309,11 +309,11 @@ def qemu_img_json(*args: str) -> Any:
return json.loads(res.stdout)
-def qemu_img_measure(*args):
-return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
+def qemu_img_measure(*args: str) -> Any:
+return qemu_img_json("measure", "--output", "json", *args)
-def qemu_img_check(*args):
-return json.loads(qemu_img_pipe("check", "--output", "json", *args))
+def qemu_img_check(*args: str) -> Any:
+return qemu_img_json("check", "--output", "json", *args)
def qemu_img_pipe(*args: str) -> str:
'''Run qemu-img and return its output'''
--
2.35.1
[PULL 06/25] python/utils: add add_visual_margin() text decoration utility
From: John Snow
>>> print(add_visual_margin(msg, width=72, name="Commit Message"))
┏━ Commit Message ━━
┃ add_visual_margin() takes a chunk of text and wraps it in a visual
┃ container that force-wraps to a specified width. An optional title
┃ label may be given, and any of the individual glyphs used to draw the
┃ box may be replaced or specified as well.
┗━━━
Signed-off-by: John Snow
Reviewed-by: Eric Blake
Acked-by: Hanna Reitz
Message-Id: <20220321201618.903471-2-js...@redhat.com>
Signed-off-by: Hanna Reitz
---
python/qemu/utils/__init__.py | 78 +++
1 file changed, 78 insertions(+)
diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
index 7f1a5138c4..b84c86d004 100644
--- a/python/qemu/utils/__init__.py
+++ b/python/qemu/utils/__init__.py
@@ -15,7 +15,10 @@
# the COPYING file in the top-level directory.
#
+import os
import re
+import shutil
+import textwrap
from typing import Optional
# pylint: disable=import-error
@@ -23,6 +26,7 @@
__all__ = (
+'add_visual_margin',
'get_info_usernet_hostfwd_port',
'kvm_available',
'list_accel',
@@ -43,3 +47,77 @@ def get_info_usernet_hostfwd_port(info_usernet_output: str)
-> Optional[int]:
if match is not None:
return int(match[1])
return None
+
+
+# pylint: disable=too-many-arguments
+def add_visual_margin(
+content: str = '',
+width: Optional[int] = None,
+name: Optional[str] = None,
+padding: int = 1,
+upper_left: str = '┏',
+lower_left: str = '┗',
+horizontal: str = '━',
+vertical: str = '┃',
+) -> str:
+"""
+Decorate and wrap some text with a visual decoration around it.
+
+This function assumes that the text decoration characters are single
+characters that display using a single monospace column.
+
+┏━ Example ━━━
+┃ This is what this function looks like with text content that's
+┃ wrapped to 66 characters. The right-hand margin is left open to
+┃ accommodate the occasional unicode character that might make
+┃ predicting the total "visual" width of a line difficult. This
+┃ provides a visual distinction that's good-enough, though.
+┗━
+
+:param content: The text to wrap and decorate.
+:param width:
+The number of columns to use, including for the decoration
+itself. The default (None) uses the the available width of the
+current terminal, or a fallback of 72 lines. A negative number
+subtracts a fixed-width from the default size. The default obeys
+the COLUMNS environment variable, if set.
+:param name: A label to apply to the upper-left of the box.
+:param padding: How many columns of padding to apply inside.
+:param upper_left: Upper-left single-width text decoration character.
+:param lower_left: Lower-left single-width text decoration character.
+:param horizontal: Horizontal single-width text decoration character.
+:param vertical: Vertical single-width text decoration character.
+"""
+if width is None or width < 0:
+avail = shutil.get_terminal_size(fallback=(72, 24))[0]
+if width is None:
+_width = avail
+else:
+_width = avail + width
+else:
+_width = width
+
+prefix = vertical + (' ' * padding)
+
+def _bar(name: Optional[str], top: bool = True) -> str:
+ret = upper_left if top else lower_left
+if name is not None:
+ret += f"{horizontal} {name} "
+
+filler_len = _width - len(ret)
+ret += f"{horizontal * filler_len}"
+return ret
+
+def _wrap(line: str) -> str:
+return os.linesep.join(
+textwrap.wrap(
+line, width=_width - padding, initial_indent=prefix,
+subsequent_indent=prefix, replace_whitespace=False,
+drop_whitespace=True, break_on_hyphens=False)
+)
+
+return os.linesep.join((
+_bar(name, top=True),
+os.linesep.join(_wrap(line) for line in content.splitlines()),
+_bar(None, top=False),
+))
--
2.35.1
[PULL 04/25] tests: Do not treat the iotests as separate meson test target anymore
From: Thomas Huth
If there is a failing iotest, the output is currently not logged to
the console anymore. To get this working again, we need to run the
meson test runner with "--print-errorlogs" (and without "--verbose"
due to a current meson bug that will be fixed here:
https://github.com/mesonbuild/meson/commit/c3f145ca2b9f5.patch ).
We could update the "meson test" call in tests/Makefile.include,
but actually it's nicer and easier if we simply do not treat the
iotests as separate test target anymore and integrate them along
with the other test suites. This has the disadvantage of not getting
the detailed progress indication there anymore, but since that was
only working right in single-threaded "make -j1" mode anyway, it's
not a huge loss right now.
Signed-off-by: Thomas Huth
Message-Id: <20220310075048.2303495-1-th...@redhat.com>
Tested-by: Hanna Reitz
Signed-off-by: Hanna Reitz
---
meson.build| 6 +++---
scripts/mtest2make.py | 4
tests/Makefile.include | 9 +
3 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/meson.build b/meson.build
index 282e7c4650..f71e1a1468 100644
--- a/meson.build
+++ b/meson.build
@@ -3,9 +3,9 @@ project('qemu', ['c'], meson_version: '>=0.59.3',
'b_staticpic=false', 'stdsplit=false'],
version: files('VERSION'))
-add_test_setup('quick', exclude_suites: ['block', 'slow', 'thorough'],
is_default: true)
-add_test_setup('slow', exclude_suites: ['block', 'thorough'], env:
['G_TEST_SLOW=1', 'SPEED=slow'])
-add_test_setup('thorough', exclude_suites: ['block'], env: ['G_TEST_SLOW=1',
'SPEED=thorough'])
+add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true)
+add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1',
'SPEED=slow'])
+add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
not_found = dependency('', required: false)
keyval = import('keyval')
diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 4d542e8aaa..304634b71e 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -101,10 +101,6 @@ def emit_suite(name, suite, prefix):
testsuites = defaultdict(Suite)
for test in introspect['tests']:
process_tests(test, targets, testsuites)
-# HACK: check-block is a separate target so that it runs with --verbose;
-# only write the dependencies
-emit_suite_deps('block', testsuites['block'], 'check')
-del testsuites['block']
emit_prolog(testsuites, 'check')
for name, suite in testsuites.items():
emit_suite(name, suite, 'check')
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e7153c8e91..b89018cdcc 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -147,16 +147,9 @@ check-acceptance: check-acceptance-deprecated-warning |
check-avocado
# Consolidated targets
-.PHONY: check-block check check-clean get-vm-images
+.PHONY: check check-clean get-vm-images
check:
-ifneq ($(.check-block.deps),)
-check: check-block
-check-block: run-ninja
- $(if $(MAKE.n),,+)$(MESON) test $(MTESTARGS) $(.mtestargs) --verbose \
- --logbase iotestslog $(call .speed.$(SPEED), block block-slow
block-thorough)
-endif
-
check-build: run-ninja
check-clean:
--
2.35.1
[PULL 05/25] tests/qemu-iotests/testrunner: Supply a test plan in TAP mode
From: Thomas Huth
Quoting the TAP specification: "The plan tells how many tests will be
run [...]. It’s a check that the test file hasn’t stopped prematurely."
That's a good idea of course, so let's support that in the iotest
testrunner, too.
Signed-off-by: Thomas Huth
Message-Id: <20220223095816.2663005-1-th...@redhat.com>
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/testrunner.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 5c207225b1..aae70a8341 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -388,6 +388,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) ->
bool:
if self.tap:
self.env.print_env('# ')
+print('1..%d' % len(tests))
else:
self.env.print_env()
--
2.35.1
[PULL 02/25] block/rbd: fix write zeroes with growing images
From: Stefano Garzarella
Commit d24f80234b ("block/rbd: increase dynamically the image size")
added a workaround to support growing images (eg. qcow2), resizing
the image before write operations that exceed the current size.
We recently added support for write zeroes and without the
workaround we can have problems with qcow2.
So let's move the resize into qemu_rbd_start_co() and do it when
the command is RBD_AIO_WRITE or RBD_AIO_WRITE_ZEROES.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2020993
Fixes: c56ac27d2a ("block/rbd: add write zeroes support")
Signed-off-by: Stefano Garzarella
Message-Id: <20220317162638.41192-1-sgarz...@redhat.com>
Signed-off-by: Hanna Reitz
---
block/rbd.c | 26 ++
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 8f183eba2a..6caf35cbba 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1107,6 +1107,20 @@ static int coroutine_fn
qemu_rbd_start_co(BlockDriverState *bs,
assert(!qiov || qiov->size == bytes);
+if (cmd == RBD_AIO_WRITE || cmd == RBD_AIO_WRITE_ZEROES) {
+/*
+ * RBD APIs don't allow us to write more than actual size, so in order
+ * to support growing images, we resize the image before write
+ * operations that exceed the current size.
+ */
+if (offset + bytes > s->image_size) {
+int r = qemu_rbd_resize(bs, offset + bytes);
+if (r < 0) {
+return r;
+}
+}
+}
+
r = rbd_aio_create_completion(&task,
(rbd_callback_t) qemu_rbd_completion_cb, &c);
if (r < 0) {
@@ -1182,18 +1196,6 @@ coroutine_fn qemu_rbd_co_pwritev(BlockDriverState *bs,
int64_t offset,
int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags)
{
-BDRVRBDState *s = bs->opaque;
-/*
- * RBD APIs don't allow us to write more than actual size, so in order
- * to support growing images, we resize the image before write
- * operations that exceed the current size.
- */
-if (offset + bytes > s->image_size) {
-int r = qemu_rbd_resize(bs, offset + bytes);
-if (r < 0) {
-return r;
-}
-}
return qemu_rbd_start_co(bs, offset, bytes, qiov, flags, RBD_AIO_WRITE);
}
--
2.35.1
[PULL 03/25] tests/qemu-iotests: Use GNU sed in two more spots where it is necessary
From: Thomas Huth
These two spots have been missed in commit 9086c7639822 ("Rework the
checks and spots using GNU sed") - they need GNU sed, too, since they
are using the "+" address form.
Signed-off-by: Thomas Huth
Message-Id: <20220309101626.637836-1-th...@redhat.com>
Reviewed-by: Eric Blake
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/common.filter | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 9790411bf0..cc9f1a5891 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -106,13 +106,13 @@ _filter_hmp()
# replace block job offset
_filter_block_job_offset()
{
-sed -e 's/, "offset": [0-9]\+,/, "offset": OFFSET,/'
+gsed -e 's/, "offset": [0-9]\+,/, "offset": OFFSET,/'
}
# replace block job len
_filter_block_job_len()
{
-sed -e 's/, "len": [0-9]\+,/, "len": LEN,/g'
+gsed -e 's/, "len": [0-9]\+,/, "len": LEN,/g'
}
# replace actual image size (depends on the host filesystem)
--
2.35.1
[PULL 01/25] tests: add (riscv virt) machine mapping to testenv
From: laokz
Some qemu-iotests(040 etc) use PCI disk to do test. Without the
mapping, RISC-V flavor use spike as default machine which has no
PCI bus, causing test failure.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/894
Signed-off-by: Kai Zhang
Message-Id:
Reviewed-by: Alistair Francis
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/testenv.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index b11e943c8a..a864c74b12 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -235,6 +235,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
('aarch64', 'virt'),
('avr', 'mega2560'),
('m68k', 'virt'),
+('riscv32', 'virt'),
+('riscv64', 'virt'),
('rx', 'gdbsim-r5f562n8'),
('tricore', 'tricore_testboard')
)
--
2.35.1
[PULL 00/25] Block patches for 7.0-rc1
The following changes since commit 330724977b10f5b92610817e8b7d1dfed122df87:
Merge tag 'pull-misc-2022-03-21' of git://repo.or.cz/qemu/armbru into staging
(2022-03-21 17:46:40 +)
are available in the Git repository at:
https://gitlab.com/hreitz/qemu.git tags/pull-block-2022-03-22
for you to fetch changes up to 48f1fcd5c87cadef331a2cd08e67e37a789e8347:
iotests/207: Filter host fingerprint (2022-03-22 10:50:10 +0100)
Block patches for 7.0-rc1:
- iotest fixes:
- Fix some iotests for riscv targets
- Use GNU sed in more places where required
- Meson-related fixes (i.e. to print errors when they occur)
- Have qemu-img calls (from Python tests) generally raise nicely
formattable exceptions on errors
- Fix iotest 207
- Allow RBD images to be growable by writing zeroes past the end of
file, fixing qcow2 on rbd
Hanna Reitz (2):
iotests.py: Filters for VM.run_job()
iotests/207: Filter host fingerprint
John Snow (18):
python/utils: add add_visual_margin() text decoration utility
python/utils: add VerboseProcessError
iotests: Remove explicit checks for qemu_img() == 0
iotests: make qemu_img raise on non-zero rc by default
iotests: fortify compare_images() against crashes
iotests: add qemu_img_json()
iotests: use qemu_img_json() when applicable
iotests: add qemu_img_info()
iotests/remove-bitmap-from-backing: use qemu_img_info()
iotests: add qemu_img_map() function
iotests: change supports_quorum to use qemu_img
iotests: replace unchecked calls to qemu_img_pipe()
iotests/149: Remove qemu_img_pipe() call
iotests: remove remaining calls to qemu_img_pipe()
iotests: use qemu_img() in has_working_luks()
iotests: replace qemu_img_log('create', ...) calls
iotests: remove qemu_img_pipe_and_status()
iotests: make qemu_img_log and img_info_log raise on error
Stefano Garzarella (1):
block/rbd: fix write zeroes with growing images
Thomas Huth (3):
tests/qemu-iotests: Use GNU sed in two more spots where it is
necessary
tests: Do not treat the iotests as separate meson test target anymore
tests/qemu-iotests/testrunner: Supply a test plan in TAP mode
laokz (1):
tests: add (riscv virt) machine mapping to testenv
meson.build | 6 +-
block/rbd.c | 26 +--
python/qemu/utils/__init__.py | 117 +++
scripts/mtest2make.py | 4 -
tests/Makefile.include| 9 +-
tests/qemu-iotests/041| 5 +-
tests/qemu-iotests/065| 7 +-
tests/qemu-iotests/149| 7 +-
tests/qemu-iotests/149.out| 21 --
tests/qemu-iotests/163| 9 +-
tests/qemu-iotests/194| 4 +-
tests/qemu-iotests/202| 4 +-
tests/qemu-iotests/203| 4 +-
tests/qemu-iotests/207| 7 +-
tests/qemu-iotests/207.out| 6 +-
tests/qemu-iotests/211| 6 +-
tests/qemu-iotests/211.out| 10 +-
tests/qemu-iotests/216| 6 +-
tests/qemu-iotests/218| 2 +-
tests/qemu-iotests/224| 11 +-
tests/qemu-iotests/228| 12 +-
tests/qemu-iotests/234| 4 +-
tests/qemu-iotests/237| 3 +-
tests/qemu-iotests/237.out| 3 -
tests/qemu-iotests/242| 7 +-
tests/qemu-iotests/255| 8 +-
tests/qemu-iotests/255.out| 4 -
tests/qemu-iotests/257| 11 +-
tests/qemu-iotests/258| 4 +-
tests/qemu-iotests/262| 2 +-
tests/qemu-iotests/266| 2 +-
tests/qemu-iotests/274| 17 +-
tests/qemu-iotests/274.out| 29 ---
tests/qemu-iotests/280| 2 +-
tests/qemu-iotests/280.out| 1 -
tests/qemu-iotests/296| 12 +-
tests/qemu-iotests/303| 2 +-
tests/qemu-iotests/310| 13 +-
tests/qemu-iotests/common.filter | 4 +-
tests/qemu-iotests/iotests.py | 196 +-
tests/qemu-iotests/testenv.py | 2 +
tests/qemu-iotests/testrunner.py | 1 +
tests/qemu-iotests/tests/block-status-cache | 14 +-
.../qemu-iotests/tests/graph-changes-while-io | 7 +-
tests/qemu-iotests/tests/image-fleecing | 10 +-
.../tests/mirror-ready-cancel-error | 6 +-
tests/qemu-iotests/tests/mirror-top-perms | 3 +-
...
Re: [PATCH v2 3/3] Use g_new() & friends where that makes obvious sense
On Tue, 15 Mar 2022 15:41:56 +0100
Markus Armbruster wrote:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Signed-off-by: Markus Armbruster
> Reviewed-by: Philippe Mathieu-Daudé
> Reviewed-by: Cédric Le Goater
> Reviewed-by: Alex Bennée
> Acked-by: Dr. David Alan Gilbert
for */i386/*
Reviewed-by: Igor Mammedov
nit:
possible miss, see below
[...]
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index cf8e500514..0731f70410 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
missed:
pfn_list = g_malloc(sizeof (*pfn_list) * nr_pfn);
> @@ -396,7 +396,7 @@ go_physmap:
>
> mr_name = memory_region_name(mr);
>
> -physmap = g_malloc(sizeof(XenPhysmap));
> +physmap = g_new(XenPhysmap, 1);
>
> physmap->start_addr = start_addr;
> physmap->size = size;
> @@ -1281,7 +1281,7 @@ static void xen_read_physmap(XenIOState *state)
> return;
>
> for (i = 0; i < num; i++) {
> -physmap = g_malloc(sizeof (XenPhysmap));
> +physmap = g_new(XenPhysmap, 1);
> physmap->phys_offset = strtoull(entries[i], NULL, 16);
> snprintf(path, sizeof(path),
> "/local/domain/0/device-model/%d/physmap/%s/start_addr",
> @@ -1410,7 +1410,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion
> **ram_memory)
> xen_pfn_t ioreq_pfn;
> XenIOState *state;
>
> -state = g_malloc0(sizeof (XenIOState));
> +state = g_new0(XenIOState, 1);
>
> state->xce_handle = xenevtchn_open(NULL, 0);
> if (state->xce_handle == NULL) {
> @@ -1463,7 +1463,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion
> **ram_memory)
> }
>
> /* Note: cpus is empty at this point in init */
> -state->cpu_by_vcpu_id = g_malloc0(max_cpus * sizeof(CPUState *));
> +state->cpu_by_vcpu_id = g_new0(CPUState *, max_cpus);
>
> rc = xen_set_ioreq_server_state(xen_domid, state->ioservid, true);
> if (rc < 0) {
> @@ -1472,7 +1472,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion
> **ram_memory)
> goto err;
> }
>
> -state->ioreq_local_port = g_malloc0(max_cpus * sizeof (evtchn_port_t));
> +state->ioreq_local_port = g_new0(evtchn_port_t, max_cpus);
[...]
Re: [PATCH v2 0/3] iotests: Check for zstd support
22.03.2022 12:48, Hanna Reitz wrote: Ping On 02.03.22 13:45, Hanna Reitz wrote: Hi, v1 cover letter: https://lists.nongnu.org/archive/html/qemu-devel/2022-02/msg04592.html We have two tests (as far as I know) that use compression_type=zstd for qcow2 but do not check whether that is actually supported. Thomas reported this for 065, but it’s also the case for 303. This series makes 303 be skipped when zstd is not compiled in, and has 065 use zlib for each of its test cases then (it was made to use zstd just to improve on coverage, so using zlib as a fallback is perfectly fine). v2: - Add the first patch so that 065 and 303 can use these new iotests.py functions to check for zstd support instead of checking for their own qemu-img create’s output - Have 065 fall back to zlib instead of skipping zstd test cases Hanna Reitz (3): iotests.py: Add supports_qcow2_zstd_compression() iotests/065: Check for zstd support iotests/303: Check for zstd support tests/qemu-iotests/065 | 24 ++-- tests/qemu-iotests/303 | 4 +++- tests/qemu-iotests/iotests.py | 21 + 3 files changed, 42 insertions(+), 7 deletions(-) Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v2 0/3] iotests: Check for zstd support
On 02/03/2022 13.45, Hanna Reitz wrote: Hi, v1 cover letter: https://lists.nongnu.org/archive/html/qemu-devel/2022-02/msg04592.html We have two tests (as far as I know) that use compression_type=zstd for qcow2 but do not check whether that is actually supported. Thomas reported this for 065, but it’s also the case for 303. This series makes 303 be skipped when zstd is not compiled in, and has 065 use zlib for each of its test cases then (it was made to use zstd just to improve on coverage, so using zlib as a fallback is perfectly fine). v2: - Add the first patch so that 065 and 303 can use these new iotests.py functions to check for zstd support instead of checking for their own qemu-img create’s output - Have 065 fall back to zlib instead of skipping zstd test cases Hanna Reitz (3): iotests.py: Add supports_qcow2_zstd_compression() iotests/065: Check for zstd support iotests/303: Check for zstd support tests/qemu-iotests/065| 24 ++-- tests/qemu-iotests/303| 4 +++- tests/qemu-iotests/iotests.py | 21 + 3 files changed, 42 insertions(+), 7 deletions(-) Thanks, this fixes the failures of 065 and 303 on my system! Series Tested-by: Thomas Huth
Re: [PATCH v2 0/3] iotests: Check for zstd support
Ping On 02.03.22 13:45, Hanna Reitz wrote: Hi, v1 cover letter: https://lists.nongnu.org/archive/html/qemu-devel/2022-02/msg04592.html We have two tests (as far as I know) that use compression_type=zstd for qcow2 but do not check whether that is actually supported. Thomas reported this for 065, but it’s also the case for 303. This series makes 303 be skipped when zstd is not compiled in, and has 065 use zlib for each of its test cases then (it was made to use zstd just to improve on coverage, so using zlib as a fallback is perfectly fine). v2: - Add the first patch so that 065 and 303 can use these new iotests.py functions to check for zstd support instead of checking for their own qemu-img create’s output - Have 065 fall back to zlib instead of skipping zstd test cases Hanna Reitz (3): iotests.py: Add supports_qcow2_zstd_compression() iotests/065: Check for zstd support iotests/303: Check for zstd support tests/qemu-iotests/065| 24 ++-- tests/qemu-iotests/303| 4 +++- tests/qemu-iotests/iotests.py | 21 + 3 files changed, 42 insertions(+), 7 deletions(-)
Re: [PATCH 0/2] iotests/207: Filter host fingerprint
On 18.03.22 13:53, Hanna Reitz wrote: Hi, Commit e3296cc796aeaf (“block: print the server key type and fingerprint on failure”) improved the verbosity of our ssh block driver's error messages for fingerprint mismatches. However, iotest 207, which tests such errors, has not been adjusted accordingly. Since the fingerprint will differ between hosts, we need to filter it (and can’t just statically adjust the reference output). The problem is that the error condition is printed by iotest.py’s VM.run_job(), so we need to pass the filter to that function. Right now, VM.run_job() doesn’t support any filters, though, so patch 1 adds a filter parameter and makes VM.run_job() use it. Patch 2 then has the fix for iotest 207. Thanks for the review, applied to my block branch. Hanna
Re: [PATCH v5 00/18] iotests: add enhanced debugging info to qemu-img failures
On 21.03.22 21:16, John Snow wrote: V5 hotfix: - Quote the subprocess.CompletedProcess[str] type annotations, Python 3.6 chokes on them at runtime :( - Reduce line length in the text decoration fn, even though check-patch still doesn't like it. - Reflow docstring for qemu_img.* V5 CI: https://gitlab.com/jsnow/qemu/-/pipelines/496050668 [*] Didn't re-run CI for this tiny change. If it breaks something I WILL cry. Don’t worry, I won’t tell you. Thanks for the quick fix! I swapped v4 for this new version in my block branch. Hanna
Re: [PATCH] block/rbd: fix write zeroes with growing images
On 21.03.22 09:31, Stefano Garzarella wrote:
On Sat, Mar 19, 2022 at 04:15:33PM +0100, Peter Lieven wrote:
Am 18.03.2022 um 17:47 schrieb Stefano Garzarella
:
On Fri, Mar 18, 2022 at 04:48:18PM +0100, Peter Lieven wrote:
Am 18.03.2022 um 09:25 schrieb Stefano Garzarella
:
On Thu, Mar 17, 2022 at 07:27:05PM +0100, Peter Lieven wrote:
Am 17.03.2022 um 17:26 schrieb Stefano Garzarella
:
Commit d24f80234b ("block/rbd: increase dynamically the image
size")
added a workaround to support growing images (eg. qcow2), resizing
the image before write operations that exceed the current size.
We recently added support for write zeroes and without the
workaround we can have problems with qcow2.
So let's move the resize into qemu_rbd_start_co() and do it when
the command is RBD_AIO_WRITE or RBD_AIO_WRITE_ZEROES.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2020993
Fixes: c56ac27d2a ("block/rbd: add write zeroes support")
Signed-off-by: Stefano Garzarella
---
block/rbd.c | 26 ++
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 8f183eba2a..6caf35cbba 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1107,6 +1107,20 @@ static int coroutine_fn
qemu_rbd_start_co(BlockDriverState *bs,
assert(!qiov || qiov->size == bytes);
+ if (cmd == RBD_AIO_WRITE || cmd == RBD_AIO_WRITE_ZEROES) {
+ /*
+ * RBD APIs don't allow us to write more than actual
size, so in order
+ * to support growing images, we resize the image
before write
+ * operations that exceed the current size.
+ */
+ if (offset + bytes > s->image_size) {
+ int r = qemu_rbd_resize(bs, offset + bytes);
+ if (r < 0) {
+ return r;
+ }
+ }
+ }
+
r = rbd_aio_create_completion(&task,
(rbd_callback_t)
qemu_rbd_completion_cb, &c);
if (r < 0) {
@@ -1182,18 +1196,6 @@ coroutine_fn
qemu_rbd_co_pwritev(BlockDriverState *bs, int64_t offset,
int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags)
{
- BDRVRBDState *s = bs->opaque;
- /*
- * RBD APIs don't allow us to write more than actual size,
so in order
- * to support growing images, we resize the image before write
- * operations that exceed the current size.
- */
- if (offset + bytes > s->image_size) {
- int r = qemu_rbd_resize(bs, offset + bytes);
- if (r < 0) {
- return r;
- }
- }
return qemu_rbd_start_co(bs, offset, bytes, qiov, flags,
RBD_AIO_WRITE);
}
--
2.35.1
Do we really have a use case for growing rbd images?
The use case is to have a qcow2 image on rbd.
I don't think it's very common, but some people use it and here
[1] we had a little discussion about features that could be
interesting (e.g. persistent dirty bitmaps for incremental backup).
In any case the support is quite simple and does not affect other
use cases since we only increase the size when we go beyond the
current size.
IMHO we can have it in :-)
The QCOW2 alone doesn’t make much sense, but additional metadata
might be a use case.
Yep.
Be aware that the current approach will serialize requests. If
there is a real use case, we might think of a better solution.
Good point, but it only happens when we have to resize, so maybe
it's okay for now, but I agree we could do better ;-)
There might also be a problem if a write for a higher offset past eof
will be executed shortly before a write to a slightly lower offset
past eof. The second resize will fail as it would shrink the image.
We would need proper locking to avoid this. Maybe we need to check if
we write past eof. If yes, take a lock around the resize op and then
check again if it’s still eof and only resize if true.
I thought rbd_resize() was synchronous. Indeed when you said this
could serialize writes it sounded like confirmation to me.
Since we call rbd_resize() before rbd_aio_writev(), I thought this
case could not occur.
Can you please elaborate?
Seconding this request, because if rbd_resize() is allowed to shrink
data, it being asynchronous might cause data corruption.
I’ll keep your patch because I find this highly unlikely, though:
qemu_rbd_resize() itself is definitely synchronous, it can’t invoke
qemu_coroutine_yield().
The only other possibility that comes to my mind is that rbd_resize()
might delay the actual resize operation, but I would still expect
consecutive resize requests to be executed in order, and since we call
rbd_aio_writev()/rbd_aio_write_zeroes() immediately after the
rbd_resize() (with no yielding in between), everything should be
executed in the order that we expect.
Hanna
