From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52367) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwCdi-0006NM-R5 for qemu-devel@nongnu.org; Wed, 03 Dec 2014 11:19:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XwCdc-0007rl-MV for qemu-devel@nongnu.org; Wed, 03 Dec 2014 11:18:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwCdc-0007rZ-Ej for qemu-devel@nongnu.org; Wed, 03 Dec 2014 11:18:48 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB3GIkdB007959 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 3 Dec 2014 11:18:47 -0500 Message-ID: <547F37E3.2050307@redhat.com> Date: Wed, 03 Dec 2014 17:18:43 +0100 From: Max Reitz MIME-Version: 1.0 References: <1417613866-25890-1-git-send-email-mreitz@redhat.com> <1417613866-25890-7-git-send-email-mreitz@redhat.com> <547F363B.6040600@redhat.com> In-Reply-To: <547F363B.6040600@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 06/26] qcow2: Use 64 bits for refcount values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 2014-12-03 at 17:11, Eric Blake wrote: > On 12/03/2014 06:37 AM, Max Reitz wrote: >> Refcounts may have a width of up to 64 bits, so qemu should use the same >> width to represent refcount values internally. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-cluster.c | 2 +- >> block/qcow2-refcount.c | 42 ++++++++++++++++++++---------------------- >> block/qcow2.h | 4 ++-- >> 3 files changed, 23 insertions(+), 25 deletions(-) >> >> @@ -1698,7 +1696,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res, >> refcount2 - refcount1, >> refcount1 > refcount2, >> QCOW2_DISCARD_ALWAYS); >> - if (ret >= 0) { >> + if (ret == 0) { >> (*num_fixed)++; >> continue; >> } > This hunk looks a bit misplaced (it is unrelated to a sizing change). Ah, right, I think it should have been in patch 3. I even looked through the patches before sending them, but somehow I missed this... > Patch 5 altered update_refcount, but does not document its return value. > But a careful step through update_refcount() shows that all error paths > return negative, and the only places where we do things like 'ret = > alloc_refcount_block(...)" are later followed by an unconditional 'ret = > 0' on success paths (so I didn't check whether alloc_refcount_block > returns positive values, but didn't need to). If you have to respin, it > might be better to add documentation of the return value and update > callers to assume a non-positive return as a separate patch, rather than > sliding it in with this one. But as I don't see any code faults, and am > not sure it is worth a respin just for this issue, Hm, I don't think this is something that should be fixed up by the applying maintainer. I will respin, even if it's just for this, but I'll wait until this series has been sufficiently looked at. > Reviewed-by: Eric Blake Once again, thank you! Max