On Thu, Nov 27, 2014 at 04:32:52PM +0100, Max Reitz wrote:
On 2014-11-27 at 16:21, Stefan Hajnoczi wrote:
On Thu, Nov 20, 2014 at 06:06:23PM +0100, Max Reitz wrote:
@@ -583,7 +608,12 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
/* we can update the count and save it */
block_index = cluster_index & (s->refcount_block_size - 1);
- refcount = be16_to_cpu(refcount_block[block_index]);
+ refcount = s->get_refcount(refcount_block, block_index);
+ if (refcount < 0) {
+ ret = -ERANGE;
+ goto fail;
+ }
Here again.
@@ -1206,11 +1236,14 @@ static int inc_refcounts(BlockDriverState *bs,
}
}
- if (++(*refcount_table)[k] == 0) {
+ refcount = s->get_refcount(*refcount_table, k);
Here the refcount < 0 check is missing. That's why it would be simpler
to eliminate the refcount < 0 case entirely.
It's not missing. This is part of the refcount check, as are all the
following ("in other places"). The refcount check builds a refcount array in
memory all by itself, so it knows for sure there are no overflows. The line
which you omitted directly after this clips the refcount values against
s->refcount_max which is INT64_MAX for 64-bit refcounts.
Therefore, no overflows possible in the refcount checking functions, because
the refcount checking functions don't introduce the overflows themselves.
The other overflow checks are only in place to reject faulty images provided
from the outside.
I see, that makes sense.
@@ -1726,7 +1761,7 @@ static void compare_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
- refcount2 = refcount_table[i];
+ refcount2 = s->get_refcount(refcount_table, i);
Missing here too and in other places.
+typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
+ uint64_t index);
Should the return type be int64_t?
No. If it was, we'd have to check for errors every time we call it, but it
cannot return errors (well, if we let it return uint64_t, it might return
-ERANGE, but that's exactly what I don't want). Therefore, let it return
uint64_t so we know this function cannot fail.
+typedef void Qcow2SetRefcountFunc(void *refcount_array,
+ uint64_t index, uint64_t value);
Should value's type be int64_t? Just because the type is unsigned
doesn't make (uint64_t)-1ULL a valid value.
Actually, it does. It's just that the implementation provided here does not
support it.
Since there is an assertion against that case in the 64-bit implementation
for this function, I don't have a problem with using int64_t here, though.
But that would break symmetry with Qcow2GetRefcountFunc(), and I do have a
reason there not to return a signed value, as explained above.
Okay, so uint64_t is really a different type - it's the qcow2 spec
64-bit refcount. int64_t is the QEMU implementation's internal
representation.
This seems error-prone to me. Maybe comments would have helped, but
it's best to eliminate the problem entirely. Why not bite the bullet
and fix up qcow2?
There are two external function prototypes that need to be changed:
int64_t qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index);
int64_t qcow2_update_cluster_refcount(BlockDriverState *bs,
int64_t cluster_index, int addend,
enum qcow2_discard_type type);
/* Returns 0 on success, -errno on failure */
int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index,
uint64_t *refcount);
int qcow2_update_cluster_refcount(BlockDriverState *bs,
int64_t cluster_index, int addend,
enum qcow2_discard_type type,
uint64_t *refcount);
Have I missed a fundamental reason why the implementation's internal
refcount type cannot be changed from int64_t to uint64_t?
It would keep the code complexity down and reduce errors.