qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V2 2/3] qapi: Change the qapi scripts to take th


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH V2 2/3] qapi: Change the qapi scripts to take their input as first argument.
Date: Thu, 27 Mar 2014 11:27:38 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0

On 03/27/2014 09:33 AM, Benoît Canet wrote:
> This patch is here to pave the way for the JSON include directive which
> will need to do include loop detection.
> 

Would also be nice to mention that it improves the error message
quality.  While 3/3 is definitely 2.1 material, 1/3 and 2/3 could
arguably be committed in 2.0 as bug fixes due to the improved errors
(but it's a stretch - personally I'm fine with saving the whole series
for 2.1, as the error messages are in the build chain only for
developers to see, and not user-visible in the end product)

> Signed-off-by: Benoit Canet <address@hidden>
> ---

>  24 files changed, 76 insertions(+), 51 deletions(-)

> 
> diff --git a/Makefile b/Makefile
> index ec74039..9bec4ff 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -236,24 +236,24 @@ gen-out-type = $(subst .,-,$(suffix $@))
>  qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py
>  
>  qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\
> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> -     $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> $(gen-out-type) -o qga/qapi-generated -p "qga-" < $<, "  GEN   $@")
> +$(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> +     $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py 
> $(SRC_PATH)/qga/qapi-schema.json $(gen-out-type) -o qga/qapi-generated -p 
> "qga-" < $<, "  GEN   $@")

While I don't mind GNU getopt's ability to reorganize options to occur
after non-options, I'm not sure it's the smartest thing to do here.
Either the input file should be a new option (as in '-i input -o
output') or you should consider ordering the command line to put the
file name after all options ('-o output input' rather than 'input -o
output').  For that matter, I feel that named options are always more
flexible than positional arguments.

That said, it looks like this script _already_ has a positional argument
(gen-out-type) occurring before options, so you aren't making it any
worse.  Therefore, if you don't respin it to take the input file name as
an option, I can live with:

Reviewed-by: Eric Blake <address@hidden>

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]