Re: [PATCH 18/18] iotests: Allow check -o data_file

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4853 bytes --]

On 29.09.19 18:39, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
>> The problem with allowing the data_file option is that you want to use a
>> different data file per image used in the test.  Therefore, we need to
>> allow patterns like -o data_file='$TEST_IMG.data_file'.
>>
>> Then, we need to filter it out from qemu-img map, qemu-img create, and
>> remove the data file in _rm_test_img.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/common.filter | 21 +++++++++++++++++++--
>>  tests/qemu-iotests/common.rc     | 10 +++++++++-
>>  2 files changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
>> index 841f7642af..a11f9a8972 100644
>> --- a/tests/qemu-iotests/common.filter
>> +++ b/tests/qemu-iotests/common.filter
>> @@ -115,7 +115,15 @@ _filter_actual_image_size()
>>  # replace driver-specific options in the "Formatting..." line
>>  _filter_img_create()
>>  {
>> -    $SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
>> +    data_file_filter=()
>> +    if echo "$IMGOPTS" | grep -q 'data_file='; then
>> +        data_file=$(echo "$IMGOPTS" | sed -e 's/.*\(data_file=[^,]*\).*/\1/' \
>> +                                          -e "s#\\\$TEST_IMG#$TEST_IMG#")
> Could you maybe add a comment on this regular expression, similar
> to what is in the commit message?
> 
>> +        data_file_filter=(-e "s# $data_file##")
> Why to use array here? A variable would work too I think.

Because I find it to be much simpler to quote correctly.

>> +    fi
>> +
>> +    $SED "${data_file_filter[@]}" \
>> +        -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
>>          -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
>>          -e "s#$TEST_DIR#TEST_DIR#g" \
>>          -e "s#$IMGFMT#IMGFMT#g" \
>> @@ -198,9 +206,18 @@ _filter_img_info()
>>  # human and json output
>>  _filter_qemu_img_map()
>>  {
>> +    data_file_filter=()
>> +    if echo "$IMGOPTS" | grep -q 'data_file='; then
>> +        data_file_pattern=$(echo "$IMGOPTS" | sed -e 's/.*data_file=\([^,]*\).*/\1/' \
>> +                                                  -e 's#\$TEST_IMG#\\(.*\\)#')
>> +        data_file_filter=(-e "s#$data_file_pattern#\\1#")
> Seems to do the same thing, but in slightly different way.

It does a very different thing, in that it creates a sed pattern.

I’d first need to get comfortable with the thought of having a common
function _data_file_from_imgopts and then pass a '\(.*\)' to it, which
would look weird.

Or maybe even a '\\(.*\\)', because I’d still have to take the sed
escaping into account, even in that function’s caller, which is weird, too.

> Maybe make a function to avoid duplication?
> I am not 100% sure though.
> 
>> +    fi
>> +
>>      $SED -e 's/\([0-9a-fx]* *[0-9a-fx]* *\)[0-9a-fx]* */\1/g' \
>>          -e 's/"offset": [0-9]\+/"offset": OFFSET/g' \
>> -        -e 's/Mapped to *//' | _filter_testdir | _filter_imgfmt
>> +        -e 's/Mapped to *//' \
>> +        "${data_file_filter[@]}" \
>> +        | _filter_testdir | _filter_imgfmt
>>  }
>>  
>>  _filter_nbd()
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index f3784077de..834ece12e4 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -297,7 +297,8 @@ _make_test_img()
>>      fi
>>  
>>      if [ -n "$IMGOPTS" ]; then
>> -        optstr=$(_optstr_add "$optstr" "$IMGOPTS")
>> +        imgopts_expanded=$(echo "$IMGOPTS" | sed -e "s#\\\$TEST_IMG#$img_name#")
>> +        optstr=$(_optstr_add "$optstr" "$imgopts_expanded")
> Also a comment explaining why this is needed would be welcome.
> 
>>      fi
>>      if [ -n "$IMGKEYSECRET" ]; then
>>          object_options="--object secret,id=keysec0,data=$IMGKEYSECRET"
>> @@ -376,6 +377,13 @@ _rm_test_img()
>>          # Remove all the extents for vmdk
>>          "$QEMU_IMG" info "$img" 2>/dev/null | grep 'filename:' | cut -f 2 -d: \
>>              | xargs -I {} rm -f "{}"
>> +    elif [ "$IMGFMT" = "qcow2" ]; then
>> +        # Remove external data file
>> +        if echo "$IMGOPTS" | grep -q 'data_file='; then
>> +            data_file=$(echo "$IMGOPTS" | sed -e 's/.*data_file=\([^,]*\).*/\1/' \
>> +                                              -e "s#\\\$TEST_IMG#$img#")
> Same here, even more evidence that it would be nice to move this to a common function.

I’m not sure whether it’s more evidence because it would increase LoC.
I may or may not do it.

Max

> Again, I am not 100% sure that this is the same regex, but it looks like that.
> 
>> +            rm -f "$data_file"
>> +        fi
>>      fi
>>      rm -f "$img"
>>  }
> 
> 
> Best regards,
> 	Maxim Levitsky
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]