Re: [PATCH v3 2/2] qemu-img: Make unallocated part of backing chain obvious in map

[Eric Blake <eblake@redhat.com>]

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 <eblake@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 <vsementsov@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