Re: [PATCH v2 2/1] qemu-img: Add "backing":true to unallocated map segments

[Kevin Wolf <kwolf@redhat.com>]

Am 23.06.2021 um 15:58 hat Nir Soffer geschrieben:
> On Wed, Jun 23, 2021 at 11:58 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 22.06.2021 um 18:56 hat Nir Soffer geschrieben:
> > > On Tue, Jun 22, 2021 at 6:38 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > > >
> > > > Am 11.06.2021 um 21:03 hat Eric Blake geschrieben:
> > > > > To save the user from having to check 'qemu-img info --backing-chain'
> > > > > or other followup command to determine which "depth":n goes beyond the
> > > > > chain, add a boolean field "backing" that is set only for unallocated
> > > > > portions of the disk.
> > > > >
> > > > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > > > ---
> > > > >
> > > > > Touches the same iotest output as 1/1.  If we decide that switching to
> > > > > "depth":n+1 is too risky, and that the mere addition of "backing":true
> > > > > while keeping "depth":n is good enough, then we'd have just one patch,
> > > > > instead of this double churn.  Preferences?
> > > >
> > > > I think the additional flag is better because it's guaranteed to be
> > > > backwards compatible, and because you don't need to know the number of
> > > > layers to infer whether a cluster was allocated in the whole backing
> > > > chain. And by exposing ALLOCATED we definitely give access to the whole
> > > > information that exists in QEMU.
> > > >
> > > > However, to continue with the bike shedding: I won't insist on
> > > > "allocated" even if that is what the flag is called internally and
> > > > consistency is usually helpful, but "backing" is misleading, too,
> > > > because intuitively it doesn't cover the top layer or standalone images
> > > > without a backing file. How about something like "present"?
> > >
> > > Looks hard to document:
> > >
> > > # @present: if present and false, the range is not allocated within the
> > > #           backing chain (since 6.1)
> >
> > I'm not sure why you would document it with a double negative.
> >
> > > And is not consistent with "offset". It would work better as:
> > >
> > > # @present: if present, the range is allocated within the backing
> > > #           chain (since 6.1)
> >
> > Completely ignoring the value? I would have documented it like this, but
> > with "if true..." instead of "if present...".
> 
> This is fine, but it means that this flag will present in all ranges,
> instead of only in unallocated ranges (what this patch is doing).

An argument for always having the flag would be that it's probably
useful for a tool to know whether a given block is actually absent or
whether it's just running an old qemu-img.

If we didn't care about this, I would still define the actual value, but
also document a default.

Kevin