Re: [PATCH 1/2] iotests: Make _filter_img_create more active

[Max Reitz]

On 17.06.20 16:02, Maxim Levitsky wrote:
> On Wed, 2020-06-17 at 15:50 +0200, Max Reitz wrote:
>> On 17.06.20 13:46, Maxim Levitsky wrote:
>>> On Tue, 2020-06-16 at 15:17 +0200, Max Reitz wrote:
>>>> Right now, _filter_img_create just filters out everything that looks
>>>> format-dependent, and applies some filename filters.  That means that we
>>>> have to add another filter line every time some format gets a new
>>>> creation option.  This can be avoided by instead discarding everything
>>>> and just keeping what we know is format-independent (format, size,
>>>> backing file, encryption information[1], preallocation) or just
>>>> interesting to have in the reference output (external data file path).
>>>>
>>>> Furthermore, we probably want to sort these options.  Format drivers are
>>>> not required to define them in any specific order, so the output is
>>>> effectively random (although this has never bothered us until now).  We
>>>> need a specific order for our reference outputs, though.  Unfortunately,
>>>> just using a plain "sort" would change a lot of existing reference
>>>> outputs, so we have to pre-filter the option keys to keep our existing
>>>> order (fmt, size, backing*, data, encryption info, preallocation).
>>>>
>>>> [1] Actually, the only thing that is really important is whether
>>>>     encryption is enabled or not.  A patch by Maxim thus removes all
>>>>     other "encrypt.*" options from the output:
>>>>     https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00339.html
>>>>     But that patch needs to come later so we can get away with changing
>>>>     as few reference outputs in this patch here as possible.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  tests/qemu-iotests/112.out       |   2 +-
>>>>  tests/qemu-iotests/153           |   9 ++-
>>>>  tests/qemu-iotests/common.filter | 100 +++++++++++++++++++++++--------
>>>>  3 files changed, 81 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
>>>> index ae0318cabe..182655dbf6 100644
>>>> --- a/tests/qemu-iotests/112.out
>>>> +++ b/tests/qemu-iotests/112.out
>>>> @@ -5,7 +5,7 @@ QA output created by 112
>>>>  qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and 
>>>> may not exceed 64 bits
>>>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>>>  qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and 
>>>> may not exceed 64 bits
>>>> -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount_bits=-1
>>>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>>>  qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and 
>>>> may not exceed 64 bits
>>>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>>>  qemu-img: TEST_DIR/t.IMGFMT: Refcount width must be a power of two and 
>>>> may not exceed 64 bits
>>>> diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
>>>> index cf961d3609..11e3d28841 100755
>>>> --- a/tests/qemu-iotests/153
>>>> +++ b/tests/qemu-iotests/153
>>>> @@ -167,11 +167,10 @@ done
>>>>  
>>>>  echo
>>>>  echo "== Creating ${TEST_IMG}.[abc] ==" | _filter_testdir
>>>> -(
>>>> -    $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
>>>> -    $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
>>>> -    $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
>>>> -) | _filter_img_create
>>>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}" | 
>>>> _filter_img_create
>>>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}" | 
>>>> _filter_img_create
>>>> +$QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b" \
>>>> +    | _filter_img_create
>>>>  
>>>>  echo
>>>>  echo "== Two devices sharing the same file in backing chain =="
>>>
>>> I guess this is done because now the filter expectes only a single
>>> qemu-img output.
>>
>> Yes, that’s right.
>>
>>> IMHO this is better anyway.
>>>
>>>> diff --git a/tests/qemu-iotests/common.filter 
>>>> b/tests/qemu-iotests/common.filter
>>>> index 03e4f71808..f104ad7a9b 100644
>>>> --- a/tests/qemu-iotests/common.filter
>>>> +++ b/tests/qemu-iotests/common.filter
>>>> @@ -122,38 +122,90 @@ _filter_actual_image_size()
>>>>  # replace driver-specific options in the "Formatting..." line
>>>>  _filter_img_create()
>>>>  {
>>>> -    data_file_filter=()
>>>> -    if data_file=$(_get_data_file "$TEST_IMG"); then
>>>> -        data_file_filter=(-e "s# data_file=$data_file##")
>>>> +    # Keep QMP output unchanged
>>>> +    qmp_pre=''
>>>> +    qmp_post=''
>>>> +    to_filter=''
>>>> +
>>>> +    while read -r line; do
>>>> +        if echo "$line" | grep -q '^{.*}$'; then
>>>> +            if [ -z "$to_filter" ]; then
>>>> +                # Use $'\n' so the newline is not dropped on variable
>>>> +                # expansion
>>>> +                qmp_pre="$qmp_pre$line"$'\n'
>>>> +            else
>>>> +                qmp_post="$qmp_post$line"$'\n'
>>>> +            fi
>>>> +        else
>>>> +            to_filter="$to_filter$line"$'\n'
>>>> +        fi
>>>> +    done
>>>
>>> The above code basically assumes that qmp output starts with '{' and ends 
>>> with '}'
>>> which I guess is fair, and then it assumes that we can have set of qmp 
>>> commands prior
>>> to qemu-img line and another set of qmp commands after it.
>>> To me it feels like we should have another filter for that, since
>>> qemu-img itself doesn't use qmp.
>>
>> Yes, but drive-backup and drive-mirror with mode=absolute-paths use
>> bdrv_img_create() (quiet=false), which emits the “Formatting” line, too.
>>  So some QMP commands may emit it and then require the same filtering as
>> qemu-img create.
> 
> Ah. Do we need this though? 
> Assuming yes, maybe we should create a new filter called something like 
> _filter_drive_backup_mirror
> or something like that that would filter qmp output, and pipe the 
> 'Formatting' line
> to _filter_img_create which then can have the qmp parsing part removed?

Hm.  I don’t think it would be that much more simpler.  (Instead of just
putting it into this function.)  But perhaps cleaner.

One of the problems is that I don’t want to actually parse JSON here.
That leaves us with the problem of what to do with line breaks.  Both
QMP stuff and any Formatting line may contain line breaks (and there
actually are iotests that have line breaks in their Formatting line).

In this function, I opted to allow line breaks in Formatting lines, but
not in QMP.  If we had a separate function specifically for Formatting
stuff in QMP, I think it should probably work the other way around.

So what this other function would do is probably to just echo all lines
that don’t start with Formatting, and filter those Formatting line(s)
through _filter_img_create.  (I don’t think we need to support
multi-line Formatting lines in QMP output.)

I think that should work.  And it’s probably indeed cleaner than putting
both into a single function.  I’ll try my hands on it.

[...]

>>>> +        | grep -ae 
>>>> "^\(fmt\\|size\\|backing\\|preallocation\\|encrypt$grep_data_file\\)" \
>>>> +        | $SED "${filename_filters[@]}" \
>>>> +            -e 's/^\(fmt\)/0-\1/' \
>>>> +            -e 's/^\(size\)/1-\1/' \
>>>> +            -e 's/^\(backing\)/2-\1/' \
>>>> +            -e 's/^\(data_file\)/3-\1/' \
>>>> +            -e 's/^\(encryption\)/4-\1/' \
>>>> +            -e 's/^\(encrypt\.format\)/5-\1/' \
>>>> +            -e 's/^\(encrypt\.key-secret\)/6-\1/' \
>>>> +            -e 's/^\(encrypt\.iter-time\)/7-\1/' \
>>>> +            -e 's/^\(preallocation\)/8-\1/' \
>>> All right, I understand this now, but do we have to do this?
>>> Maybe it is better to just update the outputs once to avoid keeping
>>> the custom sort order?
>>
>> Well.  I don’t know.  The advantage of doing this is that I can’t
>> accidentally miss updating any reference outputs that aren’t used on my
>> machine (like 026.out.nocache or 051.out).
> 
> Fair enough. A follow up patch can always be made to fix this.

Right.

>> So we don’t strictly have to do this, but I found this to be simpler.
>> If someone wants to drop this and in turn update all reference outputs,
>> they’ll be my guest, but I preferred not to do that myself.
>>
>>>> +        | sort \
>>>> +        | $SED -e 's/^[0-9]-//' \
>>>> +        | tr '\n\0' ' \n' \
>>>> +        | $SED -e 's/^ *$//' -e 's/ *$//'
>>>> +    )
>>>
>>> For the above bash pipeline overall: It was hard to decipher :-), but I 
>>> must admit
>>> I learned something from it.
>>
>> I definitely learned something while working on this, too.  I’m not sure
>> whether I also gained any useful knowledge, though. O:)
> 
> In my past experience, any knowelege eventually turns out to be useful.

In my experience, some knowledge can turn out harmful, because suddenly
you’re the expert on it. O:)

(I really don’t want to be the expert on bash.  Luckily, though, we have
Eric who fills that role quite nicely already.)

signature.asc
Description: OpenPGP digital signature