From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43871) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIj7R-0006zd-ST for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:26:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIj7M-0002iF-Fc for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:26:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34995) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIj7M-0002i7-7N for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:26:36 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t13JQZRN011302 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 3 Feb 2015 14:26:35 -0500 Date: Tue, 3 Feb 2015 20:26:32 +0100 From: Kevin Wolf Message-ID: <20150203192632.GH4488@noname.redhat.com> References: <1418647857-3589-1-git-send-email-mreitz@redhat.com> <1418647857-3589-7-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1418647857-3589-7-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 06/26] qcow2: Use 64 bits for refcount values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 15.12.2014 um 13:50 hat Max Reitz geschrieben: > 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 | 46 ++++++++++++++++++++++------------------------ > block/qcow2.h | 4 ++-- > 3 files changed, 25 insertions(+), 27 deletions(-) > @@ -897,11 +895,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > int64_t l1_table_offset, int l1_size, int addend) > { Your leaving addend an int here... > BDRVQcowState *s = bs->opaque; > - uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2; > + uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2, refcount; > bool l1_allocated = false; > int64_t old_offset, old_l2_offset; > int i, j, l1_modified = 0, nb_csectors; > - uint16_t refcount; > int ret; > > l2_table = NULL; > @@ -968,7 +965,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > if (addend != 0) { > ret = update_refcount(bs, > (offset & s->cluster_offset_mask) & ~511, > - nb_csectors * 512, abs(addend), addend < 0, > + nb_csectors * 512, imaxabs(addend), addend < 0, > QCOW2_DISCARD_SNAPSHOT); > if (ret < 0) { > goto fail; > @@ -999,7 +996,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > } > if (addend != 0) { > ret = qcow2_update_cluster_refcount(bs, > - cluster_index, abs(addend), addend < 0, > + cluster_index, imaxabs(addend), addend < 0, > QCOW2_DISCARD_SNAPSHOT); > if (ret < 0) { > goto fail; > @@ -1042,7 +1039,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > if (addend != 0) { > ret = qcow2_update_cluster_refcount(bs, l2_offset >> > s->cluster_bits, > - abs(addend), addend < 0, > + imaxabs(addend), addend < 0, > QCOW2_DISCARD_SNAPSHOT); > if (ret < 0) { > goto fail; ...but still replace abs() by imaxabs(). Did you intend to convert addend or why this change? > @@ -1658,7 +1655,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > { > BDRVQcowState *s = bs->opaque; > int64_t i; > - uint16_t refcount1, refcount2; > + uint64_t refcount1, refcount2; > int ret; > > for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) { > @@ -1687,7 +1684,8 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > num_fixed = &res->corruptions_fixed; > } > > - fprintf(stderr, "%s cluster %" PRId64 " refcount=%d reference=%d\n", > + fprintf(stderr, "%s cluster %" PRId64 " refcount=%" PRIu64 > + " reference=%" PRIu64 "\n", > num_fixed != NULL ? "Repairing" : > refcount1 < refcount2 ? "ERROR" : > "Leaked", > @@ -1695,7 +1693,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > > if (num_fixed) { > ret = update_refcount(bs, i << s->cluster_bits, 1, > - abs(refcount2 - refcount1), > + imaxabs(refcount2 - refcount1), > refcount1 > refcount2, Hope I got that right. Here's my analysis: Before: refcount{1,2} were both uint16_t. Promoted to int for the subtraction. Therefore a negative result could occur. abs() takes the absolute value and the sign is passed separately. After: refcount{1,2} are both uint64_t. No integer promotion happens, we perform an unsigned subtraction. The separate passed sign is okay. For the absolute value, there are two cases: 1. refcount2 >= refcount1: No overflow occurs, everything fine. 2. refcount2 < refcount1: (refcount2 - refcount1) wraps around, but is still an uint64_t. imaxabs() takes an intmax_t, which is signed. The conversion is implementation defined, but let's assume the obvious one. imaxabs() has two cases again: diff := refcount2 - refcount1 + UINT64_MAX a. diff > INTMAX_MAX: We get diff converted back to signed, which undoes the wraparound. The absolute value of the signed difference is: -(refcount2 - refcount1) = refcount1 - refcount2 This is what we wanted. Good. b. diff <= INTMAX_MAX: diff is again converted back to signed, however its value is unchanged because diff can be represented by intmax_t. This is a positive value, so taking the absolute value changes nothing. This is _not_ refcount1 - refcount2! I suggest using a function that calculates the absolute value of the difference of two unsigned values the naive way with an if statement. Gets us rid of the implementation defined conversion, too. > QCOW2_DISCARD_ALWAYS); > if (ret >= 0) { Kevin