Re: [Qemu-devel] [PATCH v1 1/4] qemu-iotests: refine common.config

[Eric Blake]

On 11/02/2015 11:42 PM, tu bo wrote:

> I agree with you. Adding more details as below,
> Replacing sed with awk, then it's easier to read.

Subjective opinion, but yes, I tend to agree that awk can be a pain to
learn when another tool can do the same job.

> 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 :-)

No, the rule is: if there is more than one patch, you need a cover
letter, regardless of how complicated or simple the patches are. If
there is only one patch, you don't need a cover letter, but it also
doesn't hurt if you have one.

When documenting the changes between patch versions, you can either do
it all in the cover letter, or on a patch-by-patch basis.  Or both.

> 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}}')/
> 

Your mailer botched the quoting here.

>>
>> 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,

I intentionally don't read html mail, so I can't see anything marked
red.  But yes, I missed a close /; it should have been:

sed -n "/(alias of $default_machine)/"' { s/ .*//p; q; }'

for the sed command inside $()

> 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; }') /

Again, quoting is botched, but it looks like you came up with the same fix.

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

signature.asc
Description: OpenPGP digital signature