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

[Eric Blake]

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

[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.

> 
> Suggested-By: Sascha Silbe <address@hidden>
> Reviewed-by: Sascha Silbe <address@hidden>
> Signed-off-by: Bo Tu <address@hidden>
> ---
>  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 \?'.

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

Why are you moving the | across lines?

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?

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)

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

Could shorten this to:

if [[ $default_alias_machine ]]; then

since we are already using bash.

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

signature.asc
Description: OpenPGP digital signature