Hiya!

This series effectively replaces qemu_img_pipe_and_status() with a
rewritten function named qemu_img() that raises an exception on non-zero
return code by default. By the end of the series, every last invocation
of the qemu-img binary ultimately goes through qemu_img().

The exception that this function raises includes stdout/stderr output
when the traceback is printed in a a little decorated text box so that
it stands out from the jargony Python traceback readout.

(You can test what this looks like for yourself, or at least you could,
by disabling ztsd support and then running qcow2 iotest 065.)

Negative tests are still possible in two ways:

- Passing check=False to qemu_img, qemu_img_log, or img_info_log
- Catching and handling the CalledProcessError exception at the callsite.

Here's a summary of the changes to help you update any tests you may
have in flight:

qemu_img()                   => qemu_img().returncode
qemu_img_pipe()              => qemu_img().stdout
qemu_img_pipe_and_status()   => qemu_img()
json.loads(qemu_img_pipe())) => qemu_img_json()
qemu_img_log()               => qemu_img_log()

v4:

- Combine both series into one big series again.

002/18:[0007] [FC] 'python/utils: add VerboseProcessError'
  Added a comment and a temporary var to explain the
  "elif stderr is not None" thing.
  (Kept R-Bs because I live dangerously.)

006/18:[0014] [FC] 'iotests: add qemu_img_json()'
  Made this wayyyy less stupid. Thanks Hanna.

010/18:[0010] [FC] 'iotests: add qemu_img_map() function'
  Fixed test output for 211.

018/18:[down] 'iotests: make qemu_img_log and img_info_log raise on error'
  New, enforce a good return code by default for *all*
  qemu-img calls, logged or not.

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

 python/qemu/utils/__init__.py                 | 117 ++++++++++++
 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/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/iotests.py                 | 169 +++++++++++++-----
 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 +-
 .../qemu-iotests/tests/parallels-read-bitmap  |   6 +-
 .../tests/remove-bitmap-from-backing          |  14 +-
 .../qemu-iotests/tests/stream-error-on-reset  |   4 +-
 41 files changed, 353 insertions(+), 229 deletions(-)

-- 
2.34.1

On 18.03.22 00:49, John Snow wrote:
> Hiya!
>
> This series effectively replaces qemu_img_pipe_and_status() with a
> rewritten function named qemu_img() that raises an exception on non-zero
> return code by default. By the end of the series, every last invocation
> of the qemu-img binary ultimately goes through qemu_img().
>
> The exception that this function raises includes stdout/stderr output
> when the traceback is printed in a a little decorated text box so that
> it stands out from the jargony Python traceback readout.
>
> (You can test what this looks like for yourself, or at least you could,
> by disabling ztsd support and then running qcow2 iotest 065.)
>
> Negative tests are still possible in two ways:
>
> - Passing check=False to qemu_img, qemu_img_log, or img_info_log
> - Catching and handling the CalledProcessError exception at the callsite.

Thanks!  Applied to my block branch:

https://gitlab.com/hreitz/qemu/-/commits/block

Hanna

On Fri, Mar 18, 2022 at 9:36 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 18.03.22 00:49, John Snow wrote:
> > Hiya!
> >
> > This series effectively replaces qemu_img_pipe_and_status() with a
> > rewritten function named qemu_img() that raises an exception on non-zero
> > return code by default. By the end of the series, every last invocation
> > of the qemu-img binary ultimately goes through qemu_img().
> >
> > The exception that this function raises includes stdout/stderr output
> > when the traceback is printed in a a little decorated text box so that
> > it stands out from the jargony Python traceback readout.
> >
> > (You can test what this looks like for yourself, or at least you could,
> > by disabling ztsd support and then running qcow2 iotest 065.)
> >
> > Negative tests are still possible in two ways:
> >
> > - Passing check=False to qemu_img, qemu_img_log, or img_info_log
> > - Catching and handling the CalledProcessError exception at the callsite.
>
> Thanks!  Applied to my block branch:
>
> https://gitlab.com/hreitz/qemu/-/commits/block
>
> Hanna
>

Actually, hold it -- this looks like it is causing problems with the
Gitlab CI. I need to investigate these.
https://gitlab.com/jsnow/qemu/-/pipelines/495155073/failures

... and, ugh, naturally the nice error diagnostics are suppressed here
so I can't see them. Well, there's one more thing to try and fix
somehow.

--js

On 18.03.22 22:14, John Snow wrote:
> On Fri, Mar 18, 2022 at 9:36 AM Hanna Reitz <hreitz@redhat.com> wrote:
>> On 18.03.22 00:49, John Snow wrote:
>>> Hiya!
>>>
>>> This series effectively replaces qemu_img_pipe_and_status() with a
>>> rewritten function named qemu_img() that raises an exception on non-zero
>>> return code by default. By the end of the series, every last invocation
>>> of the qemu-img binary ultimately goes through qemu_img().
>>>
>>> The exception that this function raises includes stdout/stderr output
>>> when the traceback is printed in a a little decorated text box so that
>>> it stands out from the jargony Python traceback readout.
>>>
>>> (You can test what this looks like for yourself, or at least you could,
>>> by disabling ztsd support and then running qcow2 iotest 065.)
>>>
>>> Negative tests are still possible in two ways:
>>>
>>> - Passing check=False to qemu_img, qemu_img_log, or img_info_log
>>> - Catching and handling the CalledProcessError exception at the callsite.
>> Thanks!  Applied to my block branch:
>>
>> https://gitlab.com/hreitz/qemu/-/commits/block
>>
>> Hanna
>>
> Actually, hold it -- this looks like it is causing problems with the
> Gitlab CI. I need to investigate these.
> https://gitlab.com/jsnow/qemu/-/pipelines/495155073/failures
>
> ... and, ugh, naturally the nice error diagnostics are suppressed here
> so I can't see them. Well, there's one more thing to try and fix
> somehow.

I hope this patch by Thomas fixes the logging at least:

https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg02946.html

Let’s see.

Hanna

On 21.03.22 14:14, Hanna Reitz wrote:
> On 18.03.22 22:14, John Snow wrote:
>> On Fri, Mar 18, 2022 at 9:36 AM Hanna Reitz <hreitz@redhat.com> wrote:
>>> On 18.03.22 00:49, John Snow wrote:
>>>> Hiya!
>>>>
>>>> This series effectively replaces qemu_img_pipe_and_status() with a
>>>> rewritten function named qemu_img() that raises an exception on 
>>>> non-zero
>>>> return code by default. By the end of the series, every last 
>>>> invocation
>>>> of the qemu-img binary ultimately goes through qemu_img().
>>>>
>>>> The exception that this function raises includes stdout/stderr output
>>>> when the traceback is printed in a a little decorated text box so that
>>>> it stands out from the jargony Python traceback readout.
>>>>
>>>> (You can test what this looks like for yourself, or at least you 
>>>> could,
>>>> by disabling ztsd support and then running qcow2 iotest 065.)
>>>>
>>>> Negative tests are still possible in two ways:
>>>>
>>>> - Passing check=False to qemu_img, qemu_img_log, or img_info_log
>>>> - Catching and handling the CalledProcessError exception at the 
>>>> callsite.
>>> Thanks!  Applied to my block branch:
>>>
>>> https://gitlab.com/hreitz/qemu/-/commits/block
>>>
>>> Hanna
>>>
>> Actually, hold it -- this looks like it is causing problems with the
>> Gitlab CI. I need to investigate these.
>> https://gitlab.com/jsnow/qemu/-/pipelines/495155073/failures
>>
>> ... and, ugh, naturally the nice error diagnostics are suppressed here
>> so I can't see them. Well, there's one more thing to try and fix
>> somehow.
>
> I hope this patch by Thomas fixes the logging at least:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg02946.html

So I found three issues:

1. check-patch wrongfully complains about the comment added in in 
“python/utils: add add_visual_margin() text decoration utility” that 
shows an example for how the output looks.  It complains the lines 
consisting mostly of “━━━━━━━━” were too long.  I believe that’s because 
it counts bytes, not characters.

Not fatal, i.e. doesn’t break the pipeline.  We should ignore that.

2. riscv64-debian-cross-container breaks, but that looks pre-existing.  
apt complains about some dependencies.

Also marked as allowed-to-fail, so I believe we should also just ignore 
that.  (Seems to fail on `master`, too.)

3. The rest are runs complaining about 
`subprocess.CompletedProcess[str]`.  Looks like the same issue I was 
facing for ec88eed8d14088b36a3495710368b8d1a3c33420, where I had to 
specify the type as a string.

Indeed this is fixed by something like 
https://gitlab.com/hreitz/qemu/-/commit/87615eb536bdca7babe8eb4a35fd4ea810d1da24 
.  Maybe squash that in?  (If it’s the correct way to go about this?)

Hanna

On Mon, Mar 21, 2022, 11:50 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 21.03.22 14:14, Hanna Reitz wrote:
> > On 18.03.22 22:14, John Snow wrote:
> >> On Fri, Mar 18, 2022 at 9:36 AM Hanna Reitz <hreitz@redhat.com> wrote:
> >>> On 18.03.22 00:49, John Snow wrote:
> >>>> Hiya!
> >>>>
> >>>> This series effectively replaces qemu_img_pipe_and_status() with a
> >>>> rewritten function named qemu_img() that raises an exception on
> >>>> non-zero
> >>>> return code by default. By the end of the series, every last
> >>>> invocation
> >>>> of the qemu-img binary ultimately goes through qemu_img().
> >>>>
> >>>> The exception that this function raises includes stdout/stderr output
> >>>> when the traceback is printed in a a little decorated text box so that
> >>>> it stands out from the jargony Python traceback readout.
> >>>>
> >>>> (You can test what this looks like for yourself, or at least you
> >>>> could,
> >>>> by disabling ztsd support and then running qcow2 iotest 065.)
> >>>>
> >>>> Negative tests are still possible in two ways:
> >>>>
> >>>> - Passing check=False to qemu_img, qemu_img_log, or img_info_log
> >>>> - Catching and handling the CalledProcessError exception at the
> >>>> callsite.
> >>> Thanks!  Applied to my block branch:
> >>>
> >>> https://gitlab.com/hreitz/qemu/-/commits/block
> >>>
> >>> Hanna
> >>>
> >> Actually, hold it -- this looks like it is causing problems with the
> >> Gitlab CI. I need to investigate these.
> >> https://gitlab.com/jsnow/qemu/-/pipelines/495155073/failures
> >>
> >> ... and, ugh, naturally the nice error diagnostics are suppressed here
> >> so I can't see them. Well, there's one more thing to try and fix
> >> somehow.
> >
> > I hope this patch by Thomas fixes the logging at least:
> >
> > https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg02946.html
>
> So I found three issues:
>
> 1. check-patch wrongfully complains about the comment added in in
> “python/utils: add add_visual_margin() text decoration utility” that
> shows an example for how the output looks.  It complains the lines
> consisting mostly of “━━━━━━━━” were too long.  I believe that’s because
> it counts bytes, not characters.
>
> Not fatal, i.e. doesn’t break the pipeline.  We should ignore that.
>

Agree. (Though I did shorten the lines in my re-spin to see if I could make
it shut up, but it didn't work. Ignoring it is.)


> 2. riscv64-debian-cross-container breaks, but that looks pre-existing.
> apt complains about some dependencies.
>
> Also marked as allowed-to-fail, so I believe we should also just ignore
> that.  (Seems to fail on `master`, too.)
>

Yeah, I don't think this is me.


> 3. The rest are runs complaining about
> `subprocess.CompletedProcess[str]`.  Looks like the same issue I was
> facing for ec88eed8d14088b36a3495710368b8d1a3c33420, where I had to
> specify the type as a string.
>
> Indeed this is fixed by something like
>
> https://gitlab.com/hreitz/qemu/-/commit/87615eb536bdca7babe8eb4a35fd4ea810d1da24
> .  Maybe squash that in?  (If it’s the correct way to go about this?)
>
> Hanna
>

Yep, sorry for not replying. I respun the series and tested it, but it
became "way too Saturday" for me to hit send on the respin. Will do so
today.

(Annoying: I test under python 3.6, but I didn't *run the iotests with
3.6*, which is where this problem shows up. Meh.)

On 21.03.22 17:23, John Snow wrote:
>
>
> On Mon, Mar 21, 2022, 11:50 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 21.03.22 14:14, Hanna Reitz wrote:
>     > On 18.03.22 22:14, John Snow wrote:
>     >> On Fri, Mar 18, 2022 at 9:36 AM Hanna Reitz <hreitz@redhat.com>
>     wrote:
>     >>> On 18.03.22 00:49, John Snow wrote:
>     >>>> Hiya!
>     >>>>
>     >>>> This series effectively replaces qemu_img_pipe_and_status()
>     with a
>     >>>> rewritten function named qemu_img() that raises an exception on
>     >>>> non-zero
>     >>>> return code by default. By the end of the series, every last
>     >>>> invocation
>     >>>> of the qemu-img binary ultimately goes through qemu_img().
>     >>>>
>     >>>> The exception that this function raises includes
>     stdout/stderr output
>     >>>> when the traceback is printed in a a little decorated text
>     box so that
>     >>>> it stands out from the jargony Python traceback readout.
>     >>>>
>     >>>> (You can test what this looks like for yourself, or at least you
>     >>>> could,
>     >>>> by disabling ztsd support and then running qcow2 iotest 065.)
>     >>>>
>     >>>> Negative tests are still possible in two ways:
>     >>>>
>     >>>> - Passing check=False to qemu_img, qemu_img_log, or img_info_log
>     >>>> - Catching and handling the CalledProcessError exception at the
>     >>>> callsite.
>     >>> Thanks!  Applied to my block branch:
>     >>>
>     >>> https://gitlab.com/hreitz/qemu/-/commits/block
>     >>>
>     >>> Hanna
>     >>>
>     >> Actually, hold it -- this looks like it is causing problems
>     with the
>     >> Gitlab CI. I need to investigate these.
>     >> https://gitlab.com/jsnow/qemu/-/pipelines/495155073/failures
>     >>
>     >> ... and, ugh, naturally the nice error diagnostics are
>     suppressed here
>     >> so I can't see them. Well, there's one more thing to try and fix
>     >> somehow.
>     >
>     > I hope this patch by Thomas fixes the logging at least:
>     >
>     >
>     https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg02946.html
>
>     So I found three issues:
>
>     1. check-patch wrongfully complains about the comment added in in
>     “python/utils: add add_visual_margin() text decoration utility” that
>     shows an example for how the output looks.  It complains the lines
>     consisting mostly of “━━━━━━━━” were too long.  I believe that’s
>     because
>     it counts bytes, not characters.
>
>     Not fatal, i.e. doesn’t break the pipeline.  We should ignore that.
>
>
> Agree. (Though I did shorten the lines in my re-spin to see if I could 
> make it shut up, but it didn't work. Ignoring it is.)
>
>
>     2. riscv64-debian-cross-container breaks, but that looks
>     pre-existing.
>     apt complains about some dependencies.
>
>     Also marked as allowed-to-fail, so I believe we should also just
>     ignore
>     that.  (Seems to fail on `master`, too.)
>
>
> Yeah, I don't think this is me.
>
>
>     3. The rest are runs complaining about
>     `subprocess.CompletedProcess[str]`.  Looks like the same issue I was
>     facing for ec88eed8d14088b36a3495710368b8d1a3c33420, where I had to
>     specify the type as a string.
>
>     Indeed this is fixed by something like
>     https://gitlab.com/hreitz/qemu/-/commit/87615eb536bdca7babe8eb4a35fd4ea810d1da24
>
>     .  Maybe squash that in?  (If it’s the correct way to go about this?)
>
>     Hanna
>
>
> Yep, sorry for not replying. I respun the series and tested it, but it 
> became "way too Saturday" for me to hit send on the respin. Will do so 
> today.

Great, thanks!

Hanna

On Fri, Mar 18, 2022, 9:36 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 18.03.22 00:49, John Snow wrote:
> > Hiya!
> >
> > This series effectively replaces qemu_img_pipe_and_status() with a
> > rewritten function named qemu_img() that raises an exception on non-zero
> > return code by default. By the end of the series, every last invocation
> > of the qemu-img binary ultimately goes through qemu_img().
> >
> > The exception that this function raises includes stdout/stderr output
> > when the traceback is printed in a a little decorated text box so that
> > it stands out from the jargony Python traceback readout.
> >
> > (You can test what this looks like for yourself, or at least you could,
> > by disabling ztsd support and then running qcow2 iotest 065.)
> >
> > Negative tests are still possible in two ways:
> >
> > - Passing check=False to qemu_img, qemu_img_log, or img_info_log
> > - Catching and handling the CalledProcessError exception at the callsite.
>
> Thanks!  Applied to my block branch:
>
> https://gitlab.com/hreitz/qemu/-/commits/block
>
> Hanna
>

Thanks so much!

I have more works-in-progress, but I want to be kind to your time. (And
tolerance level for Python.)

Important:

- 4 patches that switch to async qmp permanently. Almost no code, it's just
a policy thing, but it could affect iotests. Not for this freeze now, but
it'd help me a lot if you could take the time to ack it next week so I can
stage them and push forward with splitting the qmp library out of the tree.
I need to rebase and resend, which I'll do in just a bit.

Not urgent:

- Another 15ish patches for unifying qemu-io calls like i did for qemu-img
here. Stalled somewhat because I couldn't convincingly unify the qemu_io
calls that keep the pipe open, so I will probably just leave those calls
alone for now, unless I get a New Idea. Should I send them to the list and
you'll get to them whenever you get to them, or would you prefer I wait a
while?

- Another 15ish patches that split the "skip files" list for pylint/mypy
into separate skip-lists per tool and then drastically reduces their size
such that only a handful of files remain in each skiplist. Same question
here: Should I send these to the list and someone'll get to it whenever
they do, or would you prefer I wait?

Thanks again.
--js

>

On 18.03.22 16:08, John Snow wrote:
>
>
> On Fri, Mar 18, 2022, 9:36 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 18.03.22 00:49, John Snow wrote:
>     > Hiya!
>     >
>     > This series effectively replaces qemu_img_pipe_and_status() with a
>     > rewritten function named qemu_img() that raises an exception on
>     non-zero
>     > return code by default. By the end of the series, every last
>     invocation
>     > of the qemu-img binary ultimately goes through qemu_img().
>     >
>     > The exception that this function raises includes stdout/stderr
>     output
>     > when the traceback is printed in a a little decorated text box
>     so that
>     > it stands out from the jargony Python traceback readout.
>     >
>     > (You can test what this looks like for yourself, or at least you
>     could,
>     > by disabling ztsd support and then running qcow2 iotest 065.)
>     >
>     > Negative tests are still possible in two ways:
>     >
>     > - Passing check=False to qemu_img, qemu_img_log, or img_info_log
>     > - Catching and handling the CalledProcessError exception at the
>     callsite.
>
>     Thanks!  Applied to my block branch:
>
>     https://gitlab.com/hreitz/qemu/-/commits/block
>
>     Hanna
>
>
> Thanks so much!
>
> I have more works-in-progress, but I want to be kind to your time. 
> (And tolerance level for Python.)
>
> Important:
>
> - 4 patches that switch to async qmp permanently. Almost no code, it's 
> just a policy thing, but it could affect iotests. Not for this freeze 
> now, but it'd help me a lot if you could take the time to ack it next 
> week so I can stage them and push forward with splitting the qmp 
> library out of the tree. I need to rebase and resend, which I'll do in 
> just a bit.
>
> Not urgent:
>
> - Another 15ish patches for unifying qemu-io calls like i did for 
> qemu-img here. Stalled somewhat because I couldn't convincingly unify 
> the qemu_io calls that keep the pipe open, so I will probably just 
> leave those calls alone for now, unless I get a New Idea. Should I 
> send them to the list and you'll get to them whenever you get to them, 
> or would you prefer I wait a while?

I don’t mind you sending them.

> - Another 15ish patches that split the "skip files" list for 
> pylint/mypy into separate skip-lists per tool and then drastically 
> reduces their size such that only a handful of files remain in each 
> skiplist. Same question here: Should I send these to the list and 
> someone'll get to it whenever they do, or would you prefer I wait?

Same reply. :)

Hanna