Re: [RFC PATCH] kvm tools, qcow: Add support for growing refcount blocks

[Kevin Wolf <kwolf@redhat.com>]

Am 13.12.2011 04:41, schrieb lan,Tianyu:
> On 一, 2011-12-12 at 19:15 +0800, Kevin Wolf wrote:
>> Am 12.12.2011 11:58, schrieb Pekka Enberg:
>>> On Mon, 12 Dec 2011, Kevin Wolf wrote:
>>>>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
>>>>>
>>>>>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
>>>>>  	if (rft_idx >= rft->rf_size)
>>>>> -		return NULL;
>>>>> +		return (void *)-ENOSPC;
>>>>
>>>> Is this allowed style in kvm-tool? :-/
>>>
>>> It needs to use ERR_PTR() and related macros but otherwise I don't see a 
>>> big problem with it.
>>
>> Can you be sure that it never clashes with a valid allocation when you
>> use this in userspace?
>>
>> But yes, at least using appropriate functions should be required. And
>> this means that you can't only check for -ENOSPC, but you need to check
>> for all possible error codes (IS_ERR_VALUE() I guess).
> The qcow_read_refcount_block() is invoiked in the two places
> qcow_get_refcount() and update_cluster_refcount() and will only return
> NULL or -ENOSPC.
> 
> In the qcow_get_refcount(), when qcow_read_refcount_block() returned
> -ENOSPEC(no refcount block is available.), returning zero which means
> the cluster is not in use and the refcount block can be grew in the
> update_cluster_refcount().  
> 
> In the update_cluster_refcount(), when qcow_read_refcount_block()
> returned -ENOSPEC, it is necessary to grow the refcount blocks.
> 
> So the ENOSPEC should be specially dealt with.  
> 
> Does this make sense? :)

I'm not saying that your code won't work today, just that it's brittle.
If someone adds a different error return code somewhere and doesn't
check if all callers properly deal with error codes, it will break.

Kevin