[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Andrey Shinkevich
Subject: Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
Date: Mon, 13 Jan 2020 17:02:23 +0000

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.

With the best regards,
Andrey Shinkevich

reply via email to

[Prev in Thread] Current Thread [Next in Thread]