On 08/01/2020 01:11, Eric Blake wrote:
> 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 <vsement...@virtuozzo.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkev...@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...
>
If I merely move 'M' forward like ':hf:M:r:T:qU', will it be OK?
>> 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.
>
...and similar here without changing the existing code. Have I
understood you correctly?
>> + 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?
>
This is to remind a user that '-M' can be effective with the option
'--output=json' only. Do you think that a comment in the qemu-img.texi
would be enough and the restriction should be omitted here?
>> + }
>> +
>> 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.
>
Thank you Eric, I really missed to supply a comment for the new option
here and am going to put it below. Should I mention that option in
qapi/block-core.json file also with this patch of the series?
>> 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}.
>>
>
--
With the best regards,
Andrey Shinkevich
