qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info
Date: Tue, 01 Oct 2013 10:25:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130923 Thunderbird/17.0.9

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 <address@hidden>
---
  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]