Re: [Qemu-devel] [PATCH v3 07/22] qcow2: Helper function for refcount modification

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4348 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]