From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46055) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQvGh-0005TS-KH for qemu-devel@nongnu.org; Tue, 01 Oct 2013 04:25:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VQvGb-00048M-Jp for qemu-devel@nongnu.org; Tue, 01 Oct 2013 04:25:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35997) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQvGb-00047V-Bg for qemu-devel@nongnu.org; Tue, 01 Oct 2013 04:25:13 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r918PBBd020029 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 1 Oct 2013 04:25:11 -0400 Message-ID: <524A86E4.1080902@redhat.com> Date: Tue, 01 Oct 2013 10:25:08 +0200 From: Max Reitz MIME-Version: 1.0 References: <1379938162-14005-1-git-send-email-mreitz@redhat.com> <1379938162-14005-6-git-send-email-mreitz@redhat.com> <5249B183.5030301@redhat.com> In-Reply-To: <5249B183.5030301@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 5/6] qemu-iotests: Discard specific info in _img_info List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Fam Zheng , qemu-devel@nongnu.org, Stefan Hajnoczi 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 >> --- >> 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