Re: [Qemu-devel] [PATCHv3 2/2] qemu-img: add json output option to the check command

[Eric Blake]

On 01/21/2013 07:25 AM, Federico Simoncelli wrote:
> This option --output=[human|json] make qemu-img check output an human

s/make/makes/; s/an human/a human/

> or JSON representation at the choice of the user.
> 
> Signed-off-by: Federico Simoncelli <address@hidden>
> ---
>  qapi-schema.json |   46 +++++++++++
>  qemu-img-cmds.hx |    4 +-
>  qemu-img.c       |  232 +++++++++++++++++++++++++++++++++++++++--------------
>  qemu-img.texi    |    5 +-
>  4 files changed, 221 insertions(+), 66 deletions(-)
> 
> +++ b/qemu-img.c
> @@ -42,6 +42,16 @@ typedef struct img_cmd_t {
>      int (*handler)(int argc, char **argv);
>  } img_cmd_t;
>  
> +enum {
> +    OPTION_OUTPUT = 256,
> +    OPTION_BACKING_CHAIN = 257,
> +};
> +
> +typedef enum OutputFormat {
> +    OFORMAT_JSON,
> +    OFORMAT_HUMAN,
> +} OutputFormat;

I think it may be worth splitting this into two patches; one to refactor
the existing code (hoist definitions, create dump_human_image_check)
with no semantic change, and then a second to introduce the new json
handling.  But I'm not the maintainer, so I won't insist on it if others
think it is okay as-is.

> +static void dump_human_image_check(ImageCheck *check)
> +{
> +    if (!(check->corruptions || check->leaks || check->check_errors)) {
> +        printf("No errors were found on the image.\n");

Pre-existing, but I think 'in the image' sounds nicer than 'on the image'.

> @@ -424,77 +538,81 @@ static int img_check(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> +    if (output && !strcmp(output, "json")) {
> +        output_format = OFORMAT_JSON;
> +    } else if (output && !strcmp(output, "human")) {
> +        output_format = OFORMAT_HUMAN;
> +    } else if (output) {
> +        error_report("--output must be used with human or json as 
> argument.");
> +        return 1;
> +    }

Is this chunk of code something that should be reused via a helper
function, rather than copied and pasted among different subcommands?

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

signature.asc
Description: OpenPGP digital signature