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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3975 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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