| Message ID | 1379938162-14005-6-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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()
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(-)