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

From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
To: Eric Blake <eblake@redhat.com>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: "kwolf@redhat.com" <kwolf@redhat.com>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	Denis Lunev <den@virtuozzo.com>,
	"armbru@redhat.com" <armbru@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"mreitz@redhat.com" <mreitz@redhat.com>
Subject: Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
Date: Mon, 13 Jan 2020 10:30:16 +0000	[thread overview]
Message-ID: <be0bf681-5551-c5ec-e7b3-3589fb230176@virtuozzo.com> (raw)
In-Reply-To: <d87d183d-cd69-1994-11d0-5767ec72589e@redhat.com>

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 <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...
> 

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