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

[Stefan Hajnoczi]

On Thu, Dec 11, 2014 at 12:03:37PM +0100, Max Reitz wrote:
> 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.

I prefer the solution using the absolute value because it makes the code
idiot-proof for me to read :).

Thanks,
Stefan

pgpdY3VxSqtON.pgp
Description: PGP signature