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

[Nir Soffer]

On Wed, Jun 23, 2021 at 7:04 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> 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.

Good point, this is the best option. The disadvantage is a bigger output but
if you use json you don't care about the size of the output.

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