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 <[email protected]> > --- > 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
