Re: [PATCH 2/2] qcow2: dump QCOW2 metadata

[Eric Blake <eblake@redhat.com>]

On 12/27/19 5:43 AM, Andrey Shinkevich wrote:
> Let QEMU-IMG CHECK command show QCOW2 structure to inform a user about
> metadata allocations on disk. Introduce '-M'('--dump-meta') key option.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---

> +++ b/qemu-img.c
> @@ -173,6 +173,7 @@ static void QEMU_NORETURN help(void)
>              "       '-r leaks' repairs only cluster leaks, whereas '-r all' fixes all\n"
>              "       kinds of errors, with a higher risk of choosing the wrong fix or\n"
>              "       hiding corruption that has already occurred.\n"
> +           "  '-M' Dump QCOW2 metadata to stdout in JSON format.\n"

Should QCOW2 be all caps?


>   }
> @@ -701,9 +710,10 @@ static int img_check(int argc, char **argv)
>               {"object", required_argument, 0, OPTION_OBJECT},
>               {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
>               {"force-share", no_argument, 0, 'U'},
> +            {"dump-meta", no_argument, 0, 'M'},
>               {0, 0, 0, 0}
>           };
> -        c = getopt_long(argc, argv, ":hf:r:T:qU",
> +        c = getopt_long(argc, argv, ":hf:r:T:qU:M",

We are already inconsistent, but I tend to add options in alphabetical 
order, both here...

>                           long_options, &option_index);
>           if (c == -1) {
>               break;
> @@ -745,6 +755,9 @@ static int img_check(int argc, char **argv)
>           case 'U':
>               force_share = true;
>               break;
> +        case 'M':

...and here, as it is then easier to find a given option for later editing.

> +            fix |= BDRV_DUMP_META;
> +            break;
>           case OPTION_OBJECT: {
>               QemuOpts *opts;
>               opts = qemu_opts_parse_noisily(&qemu_object_opts,
> @@ -772,6 +785,11 @@ static int img_check(int argc, char **argv)
>           return 1;
>       }
>   
> +    if ((fix & BDRV_DUMP_META) && output_format != OFORMAT_JSON) {
> +        error_report("Metadata output in JSON format only");
> +        return 1;

Why this restriction?

> +    }
> +
>       if (qemu_opts_foreach(&qemu_object_opts,
>                             user_creatable_add_opts_foreach,
>                             qemu_img_object_print_help, &error_fatal)) {
> @@ -792,6 +810,15 @@ static int img_check(int argc, char **argv)
>       bs = blk_bs(blk);
>   
>       check = g_new0(ImageCheck, 1);
> +
> +    if (fix & BDRV_DUMP_META) {
> +        if (strcmp(bs->drv->format_name, "qcow2")) {
> +            error_report("Metadata output supported for QCOW2 format only");
> +            ret = -ENOTSUP;

This one makes sense, I guess - we may relax it in later versions if 
more file formats gain the ability to dump extra metadata.


> +++ b/qemu-img.texi
> @@ -230,7 +230,7 @@ specified as well.
>   For write tests, by default a buffer filled with zeros is written. This can be
>   overridden with a pattern byte specified by @var{pattern}.
>   
> -@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] @var{filename}
> +@item check [--object @var{objectdef}] [--image-opts] [-M] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] @var{filename}

This mentions that -M is valid, but has no further documentation on what 
-M means.  Without that, it's anyone's guess.

>   
>   Perform a consistency check on the disk image @var{filename}. The command can
>   output in the format @var{ofmt} which is either @code{human} or @code{json}.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org