From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VF1Ps-0007Pm-0c for qemu-devel@nongnu.org; Thu, 29 Aug 2013 08:33:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VF1Pm-0003PV-1I for qemu-devel@nongnu.org; Thu, 29 Aug 2013 08:33:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64872) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VF1Pl-0003PR-Pa for qemu-devel@nongnu.org; Thu, 29 Aug 2013 08:33:29 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7TCXTti017367 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 29 Aug 2013 08:33:29 -0400 Date: Thu, 29 Aug 2013 14:33:43 +0200 From: Kevin Wolf Message-ID: <20130829123343.GH2961@dhcp-200-207.str.redhat.com> References: <1377771363-5198-1-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1377771363-5198-1-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: > Do not try to update the refcount for zero clusters in > qcow2_update_snapshot_refcount. > > Signed-off-by: Max Reitz > --- > block/qcow2-refcount.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) Please don't forget to add a test case for v2. > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 1244693..7555242 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > for(j = 0; j < s->l2_size; j++) { > offset = be64_to_cpu(l2_table[j]); > if (offset != 0) { > + uint64_t cluster_index; > + > old_offset = offset; > offset &= ~QCOW_OFLAG_COPIED; > - if (offset & QCOW_OFLAG_COMPRESSED) { > + > + switch (qcow2_get_cluster_type(offset)) { > + case QCOW2_CLUSTER_COMPRESSED: > nb_csectors = ((offset >> s->csize_shift) & > s->csize_mask) + 1; > if (addend != 0) { > @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > } > /* compressed clusters are never modified */ > refcount = 2; > - } else { > - uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > + break; > + > + case QCOW2_CLUSTER_NORMAL: > + cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > if (addend != 0) { > refcount = update_cluster_refcount(bs, cluster_index, addend, > QCOW2_DISCARD_SNAPSHOT); > @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > ret = refcount; > goto fail; > } > + break; I think this part isn't quite right. Even zero clusters can have an allocated offset, so I think what's really needed is that the same code runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) case for the unallocated case. > + > + default: > + refcount = 0; Other places enumerate the enum values explicitly and have a default label that simply abort()s. This will help catching forgotten places when a new value is added. > } > > if (refcount == 1) { Kevin