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

[Max Reitz]

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

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

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

Now bikeshedding begins: Also, s->free_byte_offset is initialized to 0
and that is the expected value for "nothing allocated yet".  I think I'd
prefer all of the qocw2 code to use a common invalidity constant, even
thought it would make things like that more complicated.  But then we
might get into the metadata territory (how bad is it that
s->bitmap_directory_offset too is 0 when there is no directory?),
because compressed clusters are not allowed in external files, just like
metadata is not...
So my bikeshedding result is "I think it would be nice if all of the
qcow2 code made use of this constant, but it may also be pretty stupid
to enforce that now."

Max

signature.asc
Description: OpenPGP digital signature