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 <kw...@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().
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
