From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48943) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtVIc-0006qy-F6 for qemu-devel@nongnu.org; Tue, 03 Nov 2015 01:42:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtVIZ-0001tm-6l for qemu-devel@nongnu.org; Tue, 03 Nov 2015 01:42:30 -0500 Received: from e19.ny.us.ibm.com ([129.33.205.209]:58332) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtVIZ-0001tR-2Q for qemu-devel@nongnu.org; Tue, 03 Nov 2015 01:42:27 -0500 Received: from localhost by e19.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 3 Nov 2015 01:42:25 -0500 Received: from b01cxnp23034.gho.pok.ibm.com (b01cxnp23034.gho.pok.ibm.com [9.57.198.29]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 56540C9003C for ; Tue, 3 Nov 2015 01:30:33 -0500 (EST) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id tA36gMRT64291030 for ; Tue, 3 Nov 2015 06:42:22 GMT Received: from d01av05.pok.ibm.com (localhost [127.0.0.1]) by d01av05.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id tA36f8wv001536 for ; Tue, 3 Nov 2015 01:41:08 -0500 References: <1446189186-23104-1-git-send-email-tubo@linux.vnet.ibm.com> <5633AAB3.50606@redhat.com> From: tu bo Message-ID: <56385746.90404@linux.vnet.ibm.com> Date: Tue, 3 Nov 2015 14:42:14 +0800 MIME-Version: 1.0 In-Reply-To: <5633AAB3.50606@redhat.com> Content-Type: multipart/alternative; boundary="------------090403070401050507030601" Subject: Re: [Qemu-devel] [PATCH v1 1/4] qemu-iotests: refine common.config List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: kwolf@redhat.com, mreitz@redhat.com, mimu@linux.vnet.ibm.com, armbru@redhat.com, silbe@linux.vnet.ibm.com This is a multi-part message in MIME format. --------------090403070401050507030601 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi Eric: thanks for your review. On 10/31/2015 01:36 AM, Eric Blake wrote: > On 10/30/2015 01:13 AM, Bo Tu wrote: >> Be easier to read, and be slightly shorter. > You mentioned a very short "what" in the subject line (good), but the > "why" in the commit body ("easier to read, shorter") is rather terse and > subjective. It would be nicer to go into details (change the definition > of default_alias_machine) or give a sample of what changes (use grep > instead of awk). I agree with you. Adding more details as below, Replacing sed with awk, then it's easier to read. Replacing "[ ! -z "$default_alias_machine" ]" with "[[ $default_alias_machine ]]", then it's slightly shorter. > > [meta-comment] > > When sending a series, please include a 0/4 cover letter. You may want > to do: > git config format.coverLetter auto > to make it automatic when using git format-patch/send-email. My understanding is that cover letter is needed if the patch set is a little bit complicated. Cover letter is not needed if patch set just has minor change and comment is already in the git message for every patch. If my understanding above is wrong, please correct me. I just hope to be more clear about process :-) >> Suggested-By: Sascha Silbe >> Reviewed-by: Sascha Silbe >> Signed-off-by: Bo Tu >> --- >> tests/qemu-iotests/common.config | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config >> index 596bb2b..0a165e8 100644 >> --- a/tests/qemu-iotests/common.config >> +++ b/tests/qemu-iotests/common.config >> @@ -129,10 +129,9 @@ export QEMU_IO=_qemu_io_wrapper >> export QEMU_NBD=_qemu_nbd_wrapper >> >> default_machine=$($QEMU -machine \? | awk '/(default)/{print $1}') >> -default_alias_machine=$($QEMU -machine \? |\ >> - awk -v var_default_machine="$default_machine"\)\ >> - '{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print $1}}') >> -if [ ! -z "$default_alias_machine" ]; then >> +default_alias_machine=$($QEMU -machine \? \ > As long as we are touching this, we ought to switch to using '-machine > help' rather than the deprecated '-machine \?'. thanks to point this. > >> + | grep -F "(alias of ${default_machine})" |cut -d ' ' -f 1 |head -n 1) > Why are you moving the | across lines? thanks to point this > > If we are rewriting to avoid awk, why not do it with a single sed > process, rather than a grep|cut|head pipeline? For that matter, why not > rewrite default_machine to also avoid awk? The goal is not to avoid awk. The line of default_machine is easy to read, but the line of default_alias_machine as below is not easy to read, that's why Sascha suggest me to change it for the line of default_alias_machine. /-default_alias_machine=$($QEMU -machine \? |\ - awk -v var_default_machine="$default_machine"\)\ - '{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print $1}}')/ > > default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p') > default_alias_machine=$($QEMU -machine help | \ > sed -n "/(alias of $default_machine)"' { s/ .*//p; q; }') > > (which happens to work even if $default_machine contains '.', but might > get a bit dicey if the machine names could ever contain ?, [, *, or > other regex metacharacters) I like the single sed process. However, I got error message as below after running it, /sed: -e expression #1, char 38: unknown command: `.' /I guess you missed a '/' which is marked as red as below, So I change it as below, /default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p') default_alias_machine=$($QEMU -machine help | \ sed -n "/(alias of $default_machine)"/' { s/ .*//p; q; }') / > >> +if [ -n "$default_alias_machine" ]; then > Could shorten this to: > > if [[ $default_alias_machine ]]; then thanks to point this. > > since we are already using bash. > --------------090403070401050507030601 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit 
Hi Eric:
thanks for your review.
On 10/31/2015 01:36 AM, Eric Blake wrote:
On 10/30/2015 01:13 AM, Bo Tu wrote:

Be easier to read, and be slightly shorter.

You mentioned a very short "what" in the subject line (good), but the
"why" in the commit body ("easier to read, shorter") is rather terse and
subjective.  It would be nicer to go into details (change the definition
of default_alias_machine) or give a sample of what changes (use grep
instead of awk).
I agree with you. Adding more details as below, 
Replacing sed with awk, then it's easier to read.
Replacing "[ ! -z "$default_alias_machine" ]" with "[[ $default_alias_machine ]]",
then it's slightly shorter.

[meta-comment]

When sending a series, please include a 0/4 cover letter.  You may want
to do:
git config format.coverLetter auto
to make it automatic when using git format-patch/send-email.

My understanding is that cover letter is needed if the patch set is a little bit complicated.
Cover letter is not needed if patch set just has minor change and comment is already in the
git message for every patch. 
If my understanding above is wrong, please correct me. I just hope to be more clear about process :-)

      

        
Suggested-By: Sascha Silbe <silbe@linux.vnet.ibm.com>
Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Signed-off-by: Bo Tu <tubo@linux.vnet.ibm.com>
---
 tests/qemu-iotests/common.config | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 596bb2b..0a165e8 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -129,10 +129,9 @@ export QEMU_IO=_qemu_io_wrapper
 export QEMU_NBD=_qemu_nbd_wrapper
 
 default_machine=$($QEMU -machine \? | awk '/(default)/{print $1}')
-default_alias_machine=$($QEMU -machine \? |\
-    awk -v var_default_machine="$default_machine"\)\
-    '{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print $1}}')
-if [ ! -z "$default_alias_machine" ]; then
+default_alias_machine=$($QEMU -machine \? \


      

      
As long as we are touching this, we ought to switch to using '-machine
help' rather than the deprecated '-machine \?'.

    

    
thanks to point this.

    

      



      

        
+    | grep -F "(alias of ${default_machine})" |cut -d ' ' -f 1 |head -n 1)


      

      
Why are you moving the | across lines?

    

    
thanks to point this

    

      

If we are rewriting to avoid awk, why not do it with a single sed
process, rather than a grep|cut|head pipeline?  For that matter, why not
rewrite default_machine to also avoid awk?

    

    
The goal is not to avoid awk. The line of default_machine is easy to read, but the
line of default_alias_machine as below is not easy to read, that's why Sascha 
suggest me to change it for the line of default_alias_machine.

    
-default_alias_machine=$($QEMU -machine \? |\
-    awk -v var_default_machine="$default_machine"\)\
-    '{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print $1}}')

    

      

default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p')
default_alias_machine=$($QEMU -machine help | \
  sed -n "/(alias of $default_machine)"' { s/ .*//p; q; }')

(which happens to work even if $default_machine contains '.', but might
get a bit dicey if the machine names could ever contain ?, [, *, or
other regex metacharacters)

    

    
I like the single sed process. However, I got error message as below after running it,
  sed: -e expression #1, char 38: unknown command: `.'

I guess you missed a '/' which is marked as red as below, So I change it as below,
default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p')
default_alias_machine=$($QEMU -machine help | \
   sed -n "/(alias of $default_machine)"/' { s/ .*//p; q; }')


    

      



      

        
+if [ -n "$default_alias_machine" ]; then


      

      
Could shorten this to:

if [[ $default_alias_machine ]]; then

    

    
thanks to point this.

    

      

since we are already using bash.



    

    

  


--------------090403070401050507030601--