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 <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/257 | 8 +++--
tests/qemu-iotests/iotests.py | 56 ++++++++++++++++++++++++++++++-----
2 files changed, 54 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..ec4568b24a 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,49 @@ 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 +273,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 +508,9 @@ def qemu_nbd_popen(*args):
def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
'''Return True if two image files are identical'''
- return qemu_img('compare', '-f', fmt1,
- '-F', fmt2, img1, img2) == 0
+ res = qemu_img('compare', '-f', fmt1,
+ '-F', fmt2, img1, img2, check=False)
+ return res.returncode == 0
def create_image(name, size):
'''Create a fully-allocated raw image with sector markers'''
--
2.34.1On 08.03.22 02:57, John Snow wrote:
> 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.
Why not qemu_img_pipe_and_status() as the central function where all
qemu-img calls go through?
It seems like this makes qemu_img() a second version of
qemu_img_pipe_and_status(), which is a bit weird.
(Or perhaps it should actually be qemu_tool_pipe_and_status() that
receives this treatment, with qemu-io functions just passing check=False
by default.)
> 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 <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> tests/qemu-iotests/257 | 8 +++--
> tests/qemu-iotests/iotests.py | 56 ++++++++++++++++++++++++++++++-----
> 2 files changed, 54 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..ec4568b24a 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,49 @@ 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):
I wouldn’t expect these parentheses here in any other language, are they
required in Python?
> + raise VerboseProcessError(
> + subp.returncode, full_args,
> + output=subp.stdout,
> + stderr=subp.stderr,
> + )
I trust these parameters are correct, because it really sometimes seems
like Python doc doesn’t want to tell me about the arguments that
constructors take. (The only thing I found out is that `stdout` works
as an alias for `output`, so passing `output` here and reading
`self.stdout` in `VerboseProcesError` should(tm) be fine.)
Hanna
> +
> + return subp
> +
>
> def ordered_qmp(qmsg, conv_keys=True):
> # Dictionaries are not ordered prior to 3.6, therefore:
> @@ -232,7 +273,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 +508,9 @@ def qemu_nbd_popen(*args):
>
> def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
> '''Return True if two image files are identical'''
> - return qemu_img('compare', '-f', fmt1,
> - '-F', fmt2, img1, img2) == 0
> + res = qemu_img('compare', '-f', fmt1,
> + '-F', fmt2, img1, img2, check=False)
> + return res.returncode == 0
>
> def create_image(name, size):
> '''Create a fully-allocated raw image with sector markers'''
On Thu, Mar 17, 2022 at 6:25 AM Hanna Reitz <hreitz@redhat.com> wrote: > > On 08.03.22 02:57, John Snow wrote: > > + if check and subp.returncode or (subp.returncode < 0): > > I wouldn’t expect these parentheses here in any other language, are they > required in Python? > It's not required, I just find it easier to read this way. > > + raise VerboseProcessError( > > + subp.returncode, full_args, > > + output=subp.stdout, > > + stderr=subp.stderr, > > + ) > > I trust these parameters are correct, because it really sometimes seems > like Python doc doesn’t want to tell me about the arguments that > constructors take. (The only thing I found out is that `stdout` works > as an alias for `output`, so passing `output` here and reading > `self.stdout` in `VerboseProcesError` should(tm) be fine.) > >>> import subprocess >>> help(subprocess.CalledProcessError) | __init__(self, returncode, cmd, output=None, stderr=None) | Initialize self. See help(type(self)) for accurate signature. It should be fine :tm: --js
On 17.03.22 16:24, John Snow wrote: > On Thu, Mar 17, 2022 at 6:25 AM Hanna Reitz <hreitz@redhat.com> wrote: >> On 08.03.22 02:57, John Snow wrote: >>> + if check and subp.returncode or (subp.returncode < 0): >> I wouldn’t expect these parentheses here in any other language, are they >> required in Python? >> > It's not required, I just find it easier to read this way. Oh well. >>> + raise VerboseProcessError( >>> + subp.returncode, full_args, >>> + output=subp.stdout, >>> + stderr=subp.stderr, >>> + ) >> I trust these parameters are correct, because it really sometimes seems >> like Python doc doesn’t want to tell me about the arguments that >> constructors take. (The only thing I found out is that `stdout` works >> as an alias for `output`, so passing `output` here and reading >> `self.stdout` in `VerboseProcesError` should(tm) be fine.) >> >>>> import subprocess >>>> help(subprocess.CalledProcessError) > | __init__(self, returncode, cmd, output=None, stderr=None) > | Initialize self. See help(type(self)) for accurate signature. > > It should be fine :tm: The things you learn... Hanna
On 17.03.22 11:25, Hanna Reitz wrote: > On 08.03.22 02:57, John Snow wrote: >> 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. > > Why not qemu_img_pipe_and_status() as the central function where all > qemu-img calls go through? OK, I see that your follow-up series effectively does this. Still wondering why this patch here doesn’t touch qemu_img_pipe_and_status() instead. > It seems like this makes qemu_img() a second version of > qemu_img_pipe_and_status(), which is a bit weird. > > (Or perhaps it should actually be qemu_tool_pipe_and_status() that > receives this treatment, with qemu-io functions just passing > check=False by default.) (And perhaps this, but I guess qemu-io is the only other real user of qemu_tool_pipe_and_status(), so if it doesn’t care, then there’s no real reason to change that function.) Hanna
On Thu, Mar 17, 2022, 6:41 AM Hanna Reitz <hreitz@redhat.com> wrote: > On 17.03.22 11:25, Hanna Reitz wrote: > > On 08.03.22 02:57, John Snow wrote: > >> 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. > > > > Why not qemu_img_pipe_and_status() as the central function where all > > qemu-img calls go through? > > OK, I see that your follow-up series effectively does this. Still > wondering why this patch here doesn’t touch qemu_img_pipe_and_status() > instead. > Just a bad habit, I guess. It's the way I wrote this series: add a new thing, then move the old things over to use it gradually. This patchset (and the next) is pretty much the order I actually wrote it in. I do prefer the shorter name qemu_img() for this fn, tho. (I struggle a lot with the order I write not being the order most people prefer for reading. I feel like I've never quite gotten that correct. I suppose I like to work backwards: start at the code I want and work backwards until it works again.) > > It seems like this makes qemu_img() a second version of > > qemu_img_pipe_and_status(), which is a bit weird. > > > > (Or perhaps it should actually be qemu_tool_pipe_and_status() that > > receives this treatment, with qemu-io functions just passing > > check=False by default.) > > (And perhaps this, but I guess qemu-io is the only other real user of > qemu_tool_pipe_and_status(), so if it doesn’t care, then there’s no real > reason to change that function.) > Similar reasoning: I'm not actually sure I can justify the change everywhere yet. I worked through all of qemu-io, but calls to qemu-nbd and qemu itself are not yet audited. In the end, that's the goal. Working my way backwards until replacing all of these functions, yes. Sorry for my backwards brain, maybe. I felt doing it this way got us the most benefit the quickest. > Hanna > >
On 08.03.22 02:57, John Snow wrote: > 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 <jsnow@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > tests/qemu-iotests/257 | 8 +++-- > tests/qemu-iotests/iotests.py | 56 ++++++++++++++++++++++++++++++----- > 2 files changed, 54 insertions(+), 10 deletions(-) Reviewed-by: Hanna Reitz <hreitz@redhat.com>
© 2016 - 2026 Red Hat, Inc.