Re: [Qemu-devel] [PATCH V4 2/2] qemu-img: Add json output option to the info command.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On 08/22/2012 06:45 AM, Benoît Canet wrote:
> This option --output=[human|json] make qemu-img info output on
> human or JSON representation at the choice of the user.
> 

> @@ -1083,7 +1088,6 @@ out:
>      return 0;
>  }
>  
> -
>  static void dump_snapshots(BlockDriverState *bs)
>  {

Spurious whitespace change.

> +static int img_info(int argc, char **argv)
> +{
> +    int c;
> +    bool human = false, json = false;
> +    const char *filename, *fmt, *output;
> +    BlockDriverState *bs;
> +    ImageInfo *info;
>  
>      fmt = NULL;
> +    output = NULL;
>      for(;;) {
> -        c = getopt(argc, argv, "f:h");
> +        int option_index = 0;
> +        static struct option long_options[] = {
> +            {"help", no_argument, 0, 'h'},
> +            {"format", required_argument, 0, 'f'},
> +            {"output", required_argument, 0, 'm'},

I would define a constant, such as 'enum {OPTION_FORMAT=256};', so that
you don't risk future confusion...

> +            {0, 0, 0, 0}
> +        };
> +        c = getopt_long(argc, argv, "f:h",

...if we later do add an 'm' short option.

> +                        long_options, &option_index);
>          if (c == -1) {
>              break;
>          }
> @@ -1128,6 +1299,9 @@ static int img_info(int argc, char **argv)
>          case 'f':
>              fmt = optarg;
>              break;
> +        case 'm':

Given the above, this would reuse your new named value.

> @@ -1135,52 +1309,42 @@ static int img_info(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> +    if (output && !strncmp(output, "json", strlen("json"))) {

Why strncmp?  It ignores trailing garbage (as in --output=jsonoops).
Stick to strcmp.

> +        json = true;
> +    } else if (output && !strncmp(output, "human", strlen("human"))) {

And again.

> +        human = true;
> +    } else {
> +        fprintf(stderr,
> +            "Error: --output must be used with human or json as argument.\n");
> +        return 1;
> +    }

If we get here, and --output=... was not given, then both human and json
are false.  That's a problem, since...

> +
> +    if (human) {
> +        dump_human_image_info(info);
> +        dump_snapshots(bs);
>      }
> -    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
> -    if (backing_filename[0] != '\0') {
> -        bdrv_get_full_backing_filename(bs, backing_filename2,
> -                                       sizeof(backing_filename2));
> -        printf("backing file: %s", backing_filename);
> -        if (strcmp(backing_filename, backing_filename2) != 0) {
> -            printf(" (actual path: %s)", backing_filename2);
> -        }
> -        putchar('\n');
> +
> +    if (json) {
> +        collect_snapshots(bs, info);
> +        dump_json_image_info(info);
>      }
> -    dump_snapshots(bs);
> +
> +    qapi_free_ImageInfo(info);

...you will end up with no output.  You want to default to human output
for back-compat to older usage.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]