[Top][All Lists]

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

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

From: Eric Blake
Subject: Re: [PATCH 2/2] qcow2: dump QCOW2 metadata
Date: Mon, 13 Jan 2020 10:16:14 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1

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

reply via email to

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