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

[Max Reitz]

On 19.02.19 09:45, Kevin Wolf wrote:
> 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 <address@hidden>
>>> ---
>>>  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().

You mean qcow2_co_block_status()?

>                                                        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) {
>         ...
>     }

I don't think that, because it doesn't want to know the allocation
status.  It wants to know whether it can return valid map information,
which it can if (1) qcow2_get_cluster_offset() returned a valid offset,
and (2) the data is represented in plain text (i.e. not compressed or
encrypted).

OTOH maybe having a whitelist instead of a blacklist would indeed be
more safe, in a sense.  But first, this isn't a pure whitelist, because
it still has to check "!s->crypto".  Second, it isn't like allocated
zero clusters store the data the same way it's seen in the guest.  So
even the whitelist part feels a bit weird to me.

All in all, the way it is right now makes more sense to me.

>> 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.

OK.

> 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.
I don't see how an "organic growth" code base which sometimes uses 0 and
sometimes INV_OFFSET for invalid offsets is any more trustworthy, but
whatever.  I'm in a position where I don't have to learn the qcow2 code
from scratch but instead would have to review your changes, so I won't
complain further.

Max

signature.asc
Description: OpenPGP digital signature