Patchwork [v5,5/6] qemu-iotests: Discard specific info in _img_info

login
register
mail settings
Submitter Max Reitz
Date Sept. 23, 2013, 12:09 p.m.
Message ID <1379938162-14005-6-git-send-email-mreitz@redhat.com>
Download mbox | patch
Permalink /patch/277165/
State New
Headers show

Comments

Max Reitz - Sept. 23, 2013, 12:09 p.m.
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.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.rc | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)
Eric Blake - Sept. 30, 2013, 5:14 p.m.
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 ]]

> +                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; but
based on the above, you set $json_indent to contain JUST whitespace.  A
line with trailing whitespace removed will NEVER match a variable that
contains just whitespace, so this condition will never trigger.  Not to
mention the deprecated `` and non-portable use of == and -a inside [].

> +                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"

but for the JSON output case, while I'm sure that sed could probably be
coerced into stripping lines until the first line that does not have at
least as much indentation as the line containing "format-specific", the
resulting script wouldn't be very pretty to read (I couldn't quickly
produce one, at any rate - maybe Paolo has more expertise at writing
arcane sed scripts).

This patch might be easier to review if you provided a sample in the
commit message of what input is being stripped by this patch.
Max Reitz - Oct. 1, 2013, 8:25 a.m.
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

Patch

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
+                discard=1
+            elif [ "`echo "$line" | sed -e 's/^ *//'`" == '"format-specific": {' ]; then
+                discard=2
+                json_indent="`echo "$line" | sed -e 's/^\( *\).*$/\1/'`"
+            fi
+            if [ $discard == 0 ]; then
+                echo "$line"
+            elif [ $discard == 1 -a -z "$line" ]; then
+                echo
+                discard=0
+            elif [ $discard == 2 -a "`echo "$line" | sed -e 's/ *$//'`" == "${json_indent}}," ]; then
+                discard=0
+            fi
+        done
 }
 
 _get_pids_by_name()