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

[Andrey Shinkevich]

On 13/01/2020 19:16, Eric Blake wrote:
> 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.
> 
> 

Yes, I don't mind. It is easy.


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

It is not as easy and will incur much more coding. The json output is 
priority-driven format and can be converted to a human readable one with 
some other tools if needed. This option is for developers only.

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

Would you please specify the qemu-img and qapi documentation files to 
modify? Thank you.

Andrey
-- 
With the best regards,
Andrey Shinkevich