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

[Eric Blake <eblake@redhat.com>]

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 <mreitz@redhat.com>
> 
> 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