Re: [Qemu-devel] [PATCH v4 05/26] qcow2: Use unsigned addend for update_refcount()

[Max Reitz <mreitz@redhat.com>]

On 2014-12-11 at 11:58, Stefan Hajnoczi wrote:
> On Wed, Dec 03, 2014 at 02:37:25PM +0100, Max Reitz wrote:
>> @@ -530,8 +530,16 @@ found:
>>   }
>>   
>>   /* XXX: cache several refcount block clusters ? */
>> +/* In order to decrease refcounts, set @addend to the two's complement (giving a
>> + * negative value and letting the implicit cast handle it is enough) and set
>> + * @decrease to true. @decrease must be false if the refcount should be
>> + * increased. */
> The first time I read this patch I missed this quirk and thought that a
> lot of places seemed to be doing the wrong thing with addend.
>
> This is likely to cause confusion, why not make uint16_t addend truly
> unsigned and leave the sign to bool decrease, as suggested by the
> function prototype?

Because it's very easy to call it with e.g. target_refcount - 
current_refcount, and using an addition to apply the addend will always 
work.

So, the code is a bit shorter by doing this. On the other hand, I don't 
have trouble making all callers do llabs(addend) or imaxabs(addend) (if 
the absolute value is not known at compile time) and use addition or 
subtraction in this function, depending on the boolean.

Max