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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-28 at 12:26, Stefan Hajnoczi wrote:
> 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.

Sounds simple enough. I'll take a look, thanks!

Max