Re: [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]

Am 19.02.2019 um 00:13 hat Max Reitz geschrieben:
> On 31.01.19 18:55, Kevin Wolf wrote:
> > The cluster allocation code uses 0 as an invalid offset that is used in
> > case of errors or as "offset not yet determined". With external data
> > files, a host cluster offset of 0 becomes valid, though.
> > 
> > Define a constant INV_OFFSET (which is not cluster aligned and will
> > therefore never be a valid offset) that can be used for such purposes.
> > 
> > This removes the additional host_offset == 0 check that commit
> > ff52aab2df5 introduced; the confusion between an invalid offset and
> > (erroneous) allocation at offset 0 is removed with this change.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.h         |  2 ++
> >  block/qcow2-cluster.c | 59 ++++++++++++++++++++-----------------------
> >  2 files changed, 29 insertions(+), 32 deletions(-)
> 
> qcow2_get_cluster_offset() still returns 0 for unallocated clusters.
> (And qcow2_co_block_status() tests for that, so it would never report a
> valid offset for the first cluster in an externally allocated qcow2 file.)

I think the bug here is in qcow2_get_cluster_offset(). It shouldn't look
at cluster_offset, but at ret if it wants to know the allocation status.
So I think this needs to become something like:

    if ((ret == QCOW2_CLUSTER_NORMAL || ret == QCOW2_CLUSTER_ZERO_ALLOC) &&
        !s->crypto) {
        ...
    }

> qcow2_alloc_compressed_cluster_offset() should return INV_OFFSET on
> error (yeah, there are no compressed clusters in external files, but
> this seems like the right thing to do still).

Ok, makes sense.

> (And there are cases like qcow2_co_preadv(), where cluster_offset is
> initialized to 0 -- it doesn't make a difference what it's initialized
> to (it's just to silence the compiler, I suppose), but it should still
> use this new constant now.  I think.)

I don't think I would change places where cluster_offset is never used
at all or never used alone without checking the cluster type, too.

qcow2_get_cluster_offset() still returns 0 for unallocated and
non-preallocated zero clusters, and I think that's fine because it also
returns the cluster type, which is information about whether the offset
is even valid.

In theory, it shouldn't matter at all if we return 0, INV_OFFSET or 42
there, but in practice I'd bet neither money nor production images on
this. If it ain't broke, don't fix it.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]