Re: [Qemu-devel] [PATCH] qcow2: Fix fail path in realloc_refcount_block()

[Max Reitz <mreitz@redhat.com>]

On 16.03.2014 00:16, Benoît Canet wrote:
> The Saturday 15 Mar 2014 à 21:55:54 (+0100), Max Reitz wrote :
>> If qcow2_alloc_clusters() fails, new_offset and ret will both be
>> negative after the fail label, thus passing the first if condition and
>> subsequently resulting in a call of qcow2_free_clusters() with an
>> invalid (negative) offset parameter. Fix this by checking for new_offset
>> being positive instead.
>>
>> While we're at it, clean up the whole fail path. qcow2_cache_put()
>> actually can never fail, hence the return value can safely be ignored
> I would say "actually should never fail".

Hm, I'm not sure. Currently, it simply cannot fail. The assertion is 
there basically only for future code changes and to indicate to 
developers that the function is really known to succeed.

I'll try to make the sentence more specific.

>> (aside from asserting that it indeed did not fail).
>>
>> Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to
>> qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   block/qcow2-refcount.c | 20 +++++++++++---------
>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 6151148..b111319 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1440,20 +1440,22 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>>       }
>>
>>   fail:
> I don't like the label name because its real meaning is exit: it handle both
> fail and regular exits.

I'll try to reshape it into a nicer form of nested labels.

>> -    if (new_offset && (ret < 0)) {
>> -        qcow2_free_clusters(bs, new_offset, s->cluster_size,
>> -                QCOW2_DISCARD_ALWAYS);
>> -    }
>>       if (refcount_block) {
>> -        if (ret < 0) {
>> -            qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> -        } else {
>> -            ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> -        }
>> +        /* This should never fail, as it would only do so if the given refcount
>> +         * block cannot be found in the cache. As this is impossible as long as
>> +         * there are no bugs, assert the success. */
>> +        int tmp = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> +        assert(tmp == 0);
>>       }
>> +
>>       if (ret < 0) {
>> +        if (new_offset > 0) {
>> +            qcow2_free_clusters(bs, new_offset, s->cluster_size,
>> +                                QCOW2_DISCARD_OTHER);
>> +        }
>>           return ret;
>>       }
>> +
>>       return new_offset;
> The function documentation says:
> /*
>   * Allocates a new cluster for the given refcount block (represented by its
>   * offset in the image file) and copies the current content there. This function
>   * does _not_ decrement the reference count for the currently occupied cluster.
>   *
>   * This function prints an informative message to stderr on error (and returns
>   * -errno); on success, 0 is returned.
>   */
> static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>                                        uint64_t offset)
>
>
> So why are we returning new_offset on success ?

Because the comment is wrong. I'll fix it. ;-)


Thanks,

Max

>>   }
>>
>> --
>> 1.9.0
>>
>>