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

[Eric Blake <eblake@redhat.com>]

On 1/13/20 4:30 AM, Andrey Shinkevich wrote:

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

If you don't mind writing a pre-requisite patch that sorts the existing 
options, then the patch adding your option in sorted order is easy. But 
that's asking you to do extra work, which I'm not going to insist on, so 
I can also live with your patch being in any order as it is no worse 
than existing code and anyone that wants to do a cleanup patch to sort 
things has roughly the same level of effort whether or not your patch 
without sorting lands in the meantime.


>>> +    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?

Rather, why can't we come up with some sort of sane human output, so 
that we don't have to limit the flag to just --output=json?

>>> +++ 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?

Mentioning that the qapi type exists to facilitate a qemu-img option 
might not hurt. But more important is that the qemu-img documentation 
mentions what -M does; that documentation can point to the qapi docs for 
how the output will be structured when --output=json is in effect.

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