Re: [PATCH] qemu-{img,nbd}: Don't report zeroed cluster as a hole

[Eric Blake]

On Fri, Jun 11, 2021 at 08:35:01PM +0300, Nir Soffer wrote:
> On Fri, Jun 11, 2021 at 4:28 PM Eric Blake <eblake@redhat.com> wrote:
> >
> > On Fri, Jun 11, 2021 at 10:09:09AM +0200, Kevin Wolf wrote:
> > > > Yes, that might work as well.  But we didn't previously document
> > > > depth to be optional.  Removing something from output risks breaking
> > > > more downstream tools that expect it to be non-optional, compared to
> > > > providing a new value.
> > >
> > > A negative value isn't any less unexpected than a missing key. I don't
> > > think any existing tool would be able to handle it. Encoding different
> > > meanings in a single value isn't very QAPI-like either. Usually strings
> > > that are parsed are the problem, but negative integers really isn't that
> > > much different. I don't really like this solution.
> > >
> > > Leaving out the depth feels like a better suggestion to me.
> > >
> > > But anyway, this seems to only happen at the end of the backing chain.
> > > So if the backing chain consistents of n images, why not report 'depth':
> > > n + 1? So, in the above example, you would get 1. I think this has the
> > > best chances of tools actually working correctly with the new output,
> > > even though it's still not unlikely to break something.
> >
> > Ooh, I like that.  It is closer to reality - the file data really
> > comes from the next depth, even if we have no filename at that depth.
> > v2 of my patch coming up.
> 
> How do you know the number of the layer? this info is not presented in
> qemu-img map output.

qemu-img map has two output formats.

In --output=human, areas of the disk reading as zero are elided (and
this happens to include ALL areas that were not allocated anywhere in
the chain); all other areas list the filename of the element in the
chain where the data was found.  This mode also fails if compression
or encryption prevents easy access to actual data.  In other words,
it's fragile, so no one uses it for anything programmatic, even though
it's the default.

In --output=json, no file names are output.  Instead, "depth":N tells
you how deep in the backing chain you must traverse to find the data.
"depth":0 is obvious: the file you mapped (other than the bug that
this patch is fixing where we mistakenly used "depth":0 also for
unallocated regions).  If you use "backing":null to force a 1-layer
depth, then "depth":1 is unambiguous meaning the (non-present) backing
file.

Otherwise, you do have a point: "depth":1 in isolation is ambiguous
between "not allocated anywhere in this 1-element chain" and
"allocated at the first backing file in this chain of length 2 or
more".  At which point you can indeed use "qemu-img info" to determine
the backing chain depth.  How painful is that extra step?  Does it
justify the addition of a new optional "backing":true to any portion
of the file that was beyond the end of the chain (and omit that line
for all other regions, rather than printing "backing":false)?

> 
> Users will have to run "qemu-img info --backing-chain" to understand the
> output of qemu-img map.

At any rate, it should be easy enough to output an additional field,
followup patch coming soon...

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org