Re: [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
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
>>> ---
>>> 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
Re: [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
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
> > ---
> > 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
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
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 > --- > 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
[Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
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
---
block/qcow2.h | 2 ++
block/qcow2-cluster.c | 59 ---
2 files changed, 29 insertions(+), 32 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index b17bd502f5..1f87c45977 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -461,6 +461,8 @@ typedef enum QCow2MetadataOverlap {
#define REFT_OFFSET_MASK 0xfe00ULL
+#define INV_OFFSET (-1ULL)
+
static inline bool has_data_file(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 73ea0f99d6..4889c166e8 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1106,9 +1106,9 @@ static int handle_dependencies(BlockDriverState *bs,
uint64_t guest_offset,
/*
* Checks how many already allocated clusters that don't require a copy on
- * write there are at the given guest_offset (up to *bytes). If
- * *host_offset is not zero, only physically contiguous clusters beginning at
- * this host offset are counted.
+ * write there are at the given guest_offset (up to *bytes). If *host_offset is
+ * not INV_OFFSET, only physically contiguous clusters beginning at this host
+ * offset are counted.
*
* Note that guest_offset may not be cluster aligned. In this case, the
* returned *host_offset points to exact byte referenced by guest_offset and
@@ -1140,8 +1140,8 @@ static int handle_copied(BlockDriverState *bs, uint64_t
guest_offset,
trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset,
*host_offset,
*bytes);
-assert(*host_offset == 0 ||offset_into_cluster(s, guest_offset)
-== offset_into_cluster(s, *host_offset));
+assert(*host_offset == INV_OFFSET || offset_into_cluster(s, guest_offset)
+ == offset_into_cluster(s, *host_offset));
/*
* Calculate the number of clusters to look for. We stop at L2 slice
@@ -1179,7 +1179,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t
guest_offset,
goto out;
}
-if (*host_offset != 0 && !offset_matches) {
+if (*host_offset != INV_OFFSET && !offset_matches) {
*bytes = 0;
ret = 0;
goto out;
@@ -1222,10 +1222,10 @@ out:
* contain the number of clusters that have been allocated and are contiguous
* in the image file.
*
- * If *host_offset is non-zero, it specifies the offset in the image file at
- * which the new clusters must start. *nb_clusters can be 0 on return in this
- * case if the cluster at host_offset is already in use. If *host_offset is
- * zero, the clusters can be allocated anywhere in the image file.
+ * If *host_offset is not INV_OFFSET, it specifies the offset in the image file
+ * at which the new clusters must start. *nb_clusters can be 0 on return in
+ * this case if the cluster at host_offset is already in use. If *host_offset
+ * is INV_OFFSET, the clusters can be allocated anywhere in the image file.
*
* *host_offset is updated to contain the offset into the image file at which
* the first allocated cluster starts.
@@ -1244,7 +1244,7 @@ static int do_alloc_cluster_offset(BlockDriverState *bs,
uint64_t guest_offset,
/* Allocate new clusters */
trace_qcow2_cluster_alloc_phys(qemu_coroutine_self());
-if (*host_offset == 0) {
+if (*host_offset == INV_OFFSET) {
int64_t cluster_offset =
qcow2_alloc_clusters(bs, *nb_clusters * s->cluster_size);
if (cluster_offset < 0) {
@@ -1264,8 +1264,8 @@ static int do_alloc_cluster_offset(BlockDriverState *bs,
uint64_t guest_offset,
/*
* Allocates new clusters for an area that either is yet unallocated or needs a
- * copy on write. If *host_offset is non-zero, clusters are only allocated if
- * the new allocation can match the specified host offset.
+ * copy on write. If *host_offset is not INV_OFFSET, clusters are only
+ * allocated if the new allocation can match the specified host offset.
*
* Note that guest_offset may not be cluster aligned. In this case, the
* returned *host_offset points to exact byte referenced by guest_offset and
@@ -1293,7 +1293,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t
guest_offset,
int ret;
bool keep_old_clusters = false;
-uint64_t alloc_cluster_offset = 0;
+uint64_t alloc_cluster_offset = INV_OFFSET;
