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
