Re: [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info

[Max Reitz <mreitz@redhat.com>]

On 2013-09-30 19:14, Eric Blake wrote:
> On 09/23/2013 06:09 AM, Max Reitz wrote:
>> In _img_info, filter out additional information specific to the image
>> format provided by qemu-img info, since tests designed for multiple
>> image formats would produce different outputs for every image format
>> else.
> s/else/otherwise/
>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/common.rc | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 28b39e4..12d8882 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -181,12 +181,29 @@ _check_test_img()
>>   
>>   _img_info()
>>   {
>> +    discard=0
>>       $QEMU_IMG info "$@" $TEST_IMG 2>&1 | \
>>           sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
>>               -e "s#$TEST_DIR#TEST_DIR#g" \
>>               -e "s#$IMGFMT#IMGFMT#g" \
>>               -e "/^disk size:/ D" \
>> -            -e "/actual-size/ D"
>> +            -e "/actual-size/ D" | \
>> +        while IFS='' read line; do
>> +            if [ "$line" == "Format specific information:" ]; then
> [ ... == ...] is a bashism (thank goodness this is already a bash
> script); but I generally prefer you either stick to portable syntax:
>
> if [ "$line" = "Format specific information:" ]
>
> or make it obvious that you know you are using bash:
>
> if [[ $line == "Format specific information:" ]]
>
>> +                discard=1
>> +            elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then
> Use $(), not ``.
>
> This script is already a bash script; why not exploit that and avoid a fork:
>
> elif [[ $line =~ '"format-specific": {' ]]
>
>> +                discard=2
>> +                json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"
> Use $(), not ``.
>
> Exploit bash to avoid a fork:
>
> json_indent=${line%%[! ]*}
>
>> +            fi
>> +            if [ $discard == 0 ]; then
> Again, I don't like the bashism of [ == ].
>
>> +                echo "$line"
>> +            elif [ $discard == 1 -a -z "$line" ]; then
> [ ... -a ... ] is flat out non-portable.  Even when you are already
> requiring bash.  For example:
>
> [ "$str1" -a "$str2" ]
>
> gives status 0 for most pairs of non-empty strings, but could give
> status 1 for str1="!" and str2=".".  Using a bashism, on the other hand,
> is unambiguous:
>
> elif [[ $discard == 1 && ! $line ]]
Okay, I'll rewrite all these to use bash syntax.

>> +                echo
>> +                discard=0
>> +            elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then
> Huh?  If we detected json output, then compare whether the current line
> with trailing whitespace stripped is now identical to $json_indent
Take a closer look: "${json_indent}}," is not "${json_indent}", mind the 
trailing "}," ;)
Therefore, this matches when the current line is the ending brace of the 
"format-specific" structure.

>> +                discard=0
>> +            fi
>> +        done
> For the human output case, sed can already do everything your 'while
> read' loop did:
>
> sed ...
>               -e "/^disk size:/ D" \
>               -e "/actual-size/ D" \
>               -e "/Format specific information/,/^$/ D"
Oh, nice; sorry, but multiline sed regexes are something I know 
basically nothing about. I'd leave the script as it is for now, anyway 
(while implementing your remarks), because of the JSON filter and since 
I don't think this to be the bottleneck in test cases.

In your review to patch 6 you asked whether the JSON filter is actually 
necessary: Test 043 is a shell script which simply queries the JSON 
output (in addition to the human-readable version) and is valid for 
multiple image formats (qcow2 and qed), therefore the format specific 
information has to be filtered out, both for human-readable and JSON output.

Max