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 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: [PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()
On Mon, Mar 21, 2022, 11:29 AM Eric Blake wrote: > On Fri, Mar 18, 2022 at 04:36:46PM -0400, 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. > > > > (Seems like a way to have your cake and eat it too with regards to > > maintaining bisectability while also having nice mailing list patches.) > > Interesting approach; it does appear to have made reviewing a bit > easier, so thanks for trying it. > > I'll withhold actual R-b until the last squashed patch, but so far, I > haven't seen anything that causes me grief other than the lack of > bisectability that you already have documented how it will be > addressed. [less wordy - this patch is incomplete, as advertised, but > looks good] > Meta chat about QEMU patch process: I have to admit that I often "work backwards" and I prototype things by just making a function behave like how I want it to, and then I try and measure how many things broke post-hoc and use that to decide if the refactoring is even tractable. Often the slowest part of writing a series for me is breaking apart the "WIP" commit into a series of smaller steps that don't break the bisect. Sometimes this even involves a complete rewrite of an intermediate data structure to handle the in-between step. It feels like a lot of work just to delete it several commits later, sometimes. I realize giant merge commits are tough to backport, but sometimes I really just get stumped on how to not create twice as much work for myself just to arrive at an end point I've already arrived at. Of course, making things like this reviewable is a primary concern too. I'm not sure I'm an advocate of the squash-on-merge school of thought entirely, but maybe it's not so bad to use it sparingly, sometimes. I find keeping mini-commits separate can sometimes help me iterate on the design of a series quicker, too. In this case, I did try to position fixes that would work independently of the switch ahead of the pivot, but I couldn't quite get everything. Most of what's left is really just cases where the return type matters. Eh. This is definitely "Software Engineering" and not "Computer Science". Thanks for taking a look. > -- > 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 Fri, Mar 18, 2022 at 04:36:46PM -0400, 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. > > (Seems like a way to have your cake and eat it too with regards to > maintaining bisectability while also having nice mailing list patches.) Interesting approach; it does appear to have made reviewing a bit easier, so thanks for trying it. I'll withhold actual R-b until the last squashed patch, but so far, I haven't seen anything that causes me grief other than the lack of bisectability that you already have documented how it will be addressed. [less wordy - this patch is incomplete, as advertised, but looks good] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()
Rework qemu_io() to be analogous to qemu_img(); a function that requires
a return code of zero by default unless disabled explicitly.
Tests that use qemu_io():
030 040 041 044 055 056 093 124 129 132 136 148 149 151 152 163 165 205
209 219 236 245 248 254 255 257 260 264 280 298 300 302 304
image-fleecing migrate-bitmaps-postcopy-test migrate-bitmaps-test
migrate-during-backup migration-permissions
Test that use qemu_io_log():
242 245 255 274 303 307 nbd-reconnect-on-open
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.
(Seems like a way to have your cake and eat it too with regards to
maintaining bisectability while also having nice mailing list patches.)
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]:
+"""
+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]:
+result = qemu_io(*args, check=False)
+log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
return result
def qemu_io_silent(*args):
--
2.34.1
