Re: [PATCH v2 2/3] iotests: Include QMP input in .out files

[Eric Blake]

On 10/17/19 7:59 AM, Max Reitz wrote:
> On 15.10.19 21:35, Eric Blake wrote:
> > We generally include relevant HMP input in .out files, by virtue of
> > the fact that HMP echoes its input.  But QMP does not, so we have to
> > explicitly inject it in the output stream, in order to make it easier
> > to read .out files to see what behavior is being tested (especially
> > true where the output file is a sequence of {'return': {}}).
> >
> > Suggested-by: Max Reitz <address@hidden>
>
> That was actually not my intention. :-)
>
> I was thinking of a new parameter that enables this behavior and is
> disabled by default so that existing tests don’t change.
>
> But then again I did see that you interpreted my suggestion in a
> slightly different way, and thought this is probably better, actually.

I'm glad you like how it turned out.  Now to fix the problems ;)


> > +++ b/tests/qemu-iotests/common.qemu
> > @@ -123,6 +123,9 @@ _timed_wait_for()
> >   # until either timeout, or a response.  If it is not set, or <=0,
> >   # then the command is only sent once.
> >   #
> > +# If neither $silent nor $mismatch_only is set, and $cmd begins with '{',
> > +# echo the command before sending it the first time.
> > +#
> >   # If $qemu_error_no_exit is set, then even if the expected response
> >   # is not seen, we will not exit.  $QEMU_STATUS[$1] will be set it -1 in
> >   # that case.
> > @@ -152,6 +155,12 @@ _send_qemu_cmd()
> >           shift $(($# - 2))
> >       fi
> >
> > +    # Display QMP being sent, but not HMP (since HMP already echoes its
> > +    # input back to output); decide based on leading '{'
> > +    if [ -z "$silent" ] && [ -z "$mismatch_only" ] &&
> > +            [ "$cmd" != "${cmd#{}" ]; then
>
> It’s a shame that this breaks syntax highlighting in (my) vim.  (Also I
> have to admit googling to understand ${cmd#{} wasn’t trivial.)
>
> Can I persuade you to use "${cmd#\{}" instead?  That seems to work for me.

Yes.  That, or "${cmd#'{'}" should also work.

> > diff --git a/tests/qemu-iotests/094.out b/tests/qemu-iotests/094.out
> > index f3b9ecf22b73..f3e1a9ecf736 100644
> > --- a/tests/qemu-iotests/094.out
> > +++ b/tests/qemu-iotests/094.out
> > @@ -1,16 +1,20 @@
> >   QA output created by 094
> >   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> >   Formatting 'TEST_DIR/source.IMGFMT', fmt=IMGFMT size=67108864
> > +{'execute': 'qmp_capabilities'}
> >   {"return": {}}
> > +{'execute': 'drive-mirror', 'arguments': {'device': 'src', 'target': 
> > 'nbd:127.0.0.1:10810', 'format': 'nbd', 'sync':'full', 'mode':'existing'}}
>
> This reminds me that we need to fix nbd’s $TEST_IMG to not be fixed to
> port 10810.  I get intermittent failures because of that.

And I should therefore fix the filter to display it as something more 
stable (perhaps nbd:HOST:PORT).  But I also agree that the hard-coded 
value is pre-existing broken, so a separate patch here to improve it is 
warranted.

> [...]
>
> > diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> > index 67fe44a3e390..3857675f7ebd 100644
> > --- a/tests/qemu-iotests/140.out
> > +++ b/tests/qemu-iotests/140.out
> > @@ -2,14 +2,19 @@ QA output created by 140
> >   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
> >   wrote 65536/65536 bytes at offset 0
> >   64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +{ 'execute': 'qmp_capabilities' }
> >   {"return": {}}
> > +{ 'execute': 'nbd-server-start', 'arguments': { 'addr': { 'type': 'unix', 
> > 'data': { 'path': 'TEST_DIR/nbd' }}}}
>
> Hmmmmm, this conflicts with my SOCK_DIR series.  common.qemu would then
> also need a SOCK_DIR filter.  Well, or 140 should filter it (and the
> other tests that are concerned).  I’m not 100 % sure, but a SOCK_DIR
> filter in common.qemu probably can’t hurt.

Agreed. I will rebase a v3 on top of your pending series.


> > +++ b/tests/qemu-iotests/141.out
> > @@ -2,82 +2,108 @@ QA output created by 141
> >   Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=1048576
> >   Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=1048576 
> > backing_file=TEST_DIR/b.IMGFMT
> >   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
> > backing_file=TEST_DIR/m.IMGFMT
> > +{'execute': 'qmp_capabilities'}
> >   {"return": {}}
> >
> >   === Testing drive-backup ===
> >
> > +{'execute': 'blockdev-add', 'arguments': { 'node-name': 'drv0', 'driver': 
> > 'qcow2', 'file': { 'driver': 'file', 'filename': 'TEST_DIR/t.qcow2' }}}
>
> 141 also supports qed, so this then results in a mismatch.  I suppose
> common.qemu should filter the image format.
>
> (Same for 156, 161, and 229.)

Yep, I'll have to improve the filtering.  I'll make sure I run -qed 
tests before posting v3.

> [...]
>
> > diff --git a/tests/qemu-iotests/156.out b/tests/qemu-iotests/156.out
> > index 4c391a760371..d1865044f81a 100644
> > --- a/tests/qemu-iotests/156.out
> > +++ b/tests/qemu-iotests/156.out
> > @@ -5,21 +5,27 @@ wrote 262144/262144 bytes at offset 0
> >   256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >   wrote 196608/196608 bytes at offset 65536
> >   192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +{ 'execute': 'qmp_capabilities' }
> >   {"return": {}}
> >   Formatting 'TEST_DIR/t.IMGFMT.overlay', fmt=IMGFMT size=1048576 
> > backing_file=TEST_DIR/t.IMGFMT
> > +{ 'execute': 'blockdev-snapshot-sync', 'arguments': { 'device': 'source', 
> > 'snapshot-file': 'TEST_DIR/t.qcow2.overlay', 'format': 'qcow2', 'mode': 
> > 'existing' } }
>
> Same here (as said above), although there’s also the fact to consider
> that 156 supports generic protocols.  I hope _filter_testdir handles
> that, though.

or _filter_imgfmt.  It should not be hard to turn on extra filters, such 
that this looks more like:

'snapshot-file': 'TEST_DIR/t.IMGFMT.overlay', 'format': 'IMGFMT', ...


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org