On Sat, Jul 03, 2021 at 10:25:28AM +0300, Vladimir Sementsov-Ogievskiy wrote:
...
> > An obvious solution is to make 'qemu-img map --output=json' add an
> > additional "present":false designation to any cluster lacking an
> > allocation anywhere in the chain, without any change to the "depth"
> > parameter to avoid breaking existing clients. The iotests have
> > several examples where this distinction demonstrates the additional
> > accuracy.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> > +++ b/docs/tools/qemu-img.rst
> > @@ -597,6 +597,9 @@ Command description:
> > if false, the sectors are either unallocated or stored as optimized
> > all-zero clusters);
> > - whether the data is known to read as zero (boolean field ``zero``);
> > + - whether the data is actually present (boolean field ``present``);
> > + if false, rebasing the backing chain onto a deeper file would pick
> > + up data from the deeper file;
>
> Preexisting, but rather strange style of documentation, when described option
> doesn't go first in the paragraph..
Yeah. I'll send a followup email with a rewording of those paragraphs
for consideration.
> > +++ b/qemu-img.c
> > @@ -2980,8 +2980,9 @@ static int dump_map_entry(OutputFormat output_format,
> > MapEntry *e,
> > break;
> > case OFORMAT_JSON:
> > printf("{ \"start\": %"PRId64", \"length\": %"PRId64","
> > - " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
> > - e->start, e->length, e->depth,
> > + " \"depth\": %"PRId64", \"present\": %s, \"zero\": %s,"
> > + "\"data\": %s", e->start, e->length, e->depth,
> > + e->present ? "true" : "false",
>
> Didn't you want to put present at the end? Still, this shouldn't be
> significant. And it make sense to keep present, zero and data together.
I wanted it before anything optional, which "offset" is, so it already
can't be at the end. If I understood Nir correctly, it was more
important to always be present (it's easy to write a parser that
searches for terms in the same position, and tolerates a missing term
from an older verseion, but harder to parse a term that might or might
not be present).
>
> You missied a whitespace after '"zero": %s,', which is obvious from further
> test diff hunks.
>
> With it fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
Thanks for catching that. I've updated that, and will queue through
my NBD tree.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
