From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56900) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xz1XA-0003Rp-7P for qemu-devel@nongnu.org; Thu, 11 Dec 2014 06:03:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xz1X4-0003Zu-1t for qemu-devel@nongnu.org; Thu, 11 Dec 2014 06:03:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58225) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xz1X3-0003Zl-Q6 for qemu-devel@nongnu.org; Thu, 11 Dec 2014 06:03:41 -0500 Message-ID: <54897A09.1070606@redhat.com> Date: Thu, 11 Dec 2014 12:03:37 +0100 From: Max Reitz MIME-Version: 1.0 References: <1417613866-25890-1-git-send-email-mreitz@redhat.com> <1417613866-25890-6-git-send-email-mreitz@redhat.com> <20141211105839.GE30812@stefanha-thinkpad.redhat.com> In-Reply-To: <20141211105839.GE30812@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 05/26] qcow2: Use unsigned addend for update_refcount() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi 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