From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIyJl-0003mV-9q for qemu-devel@nongnu.org; Wed, 04 Feb 2015 06:40:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIyJg-0005J9-7F for qemu-devel@nongnu.org; Wed, 04 Feb 2015 06:40:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48768) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIyJg-0005Ij-16 for qemu-devel@nongnu.org; Wed, 04 Feb 2015 06:40:20 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t14BeIZT024848 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Feb 2015 06:40:18 -0500 Date: Wed, 4 Feb 2015 12:40:15 +0100 From: Kevin Wolf Message-ID: <20150204114015.GA5641@noname.redhat.com> References: <1418647857-3589-1-git-send-email-mreitz@redhat.com> <1418647857-3589-8-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-8-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 07/26] qcow2: Respect error in qcow2_alloc_bytes() 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: > qcow2_update_cluster_refcount() may fail, and qcow2_alloc_bytes() should > mind that case. > > Signed-off-by: Max Reitz > Reviewed-by: Eric Blake > Reviewed-by: Stefan Hajnoczi > --- > block/qcow2-refcount.c | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 0308a7e..db81647 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -778,8 +778,8 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset, > int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) > { > BDRVQcowState *s = bs->opaque; > - int64_t offset, cluster_offset; > - int free_in_cluster; > + int64_t offset, cluster_offset, new_cluster; > + int free_in_cluster, ret; > > BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES); > assert(size > 0 && size <= s->cluster_size); > @@ -800,23 +800,32 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) > free_in_cluster -= size; > if (free_in_cluster == 0) > s->free_byte_offset = 0; > - if (offset_into_cluster(s, offset) != 0) > - qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, > - false, QCOW2_DISCARD_NEVER); > + if (offset_into_cluster(s, offset) != 0) { > + ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, > + 1, false, QCOW2_DISCARD_NEVER); > + if (ret < 0) { > + return ret; Not sure how relevant it is, but s->free_byte_offset has already been increased, so we're leaving sub-cluster space unused. (It's not really leaking as freeing all references still frees the cluster.) > + } > + } > } else { > - offset = qcow2_alloc_clusters(bs, s->cluster_size); > - if (offset < 0) { > - return offset; > + new_cluster = qcow2_alloc_clusters(bs, s->cluster_size); > + if (new_cluster < 0) { > + return new_cluster; > } offset is the return value of this function, and now there are cases where it isn't set to new_cluster any more (I wonder why gcc doesn't warn). Why can't we keep offset where it was used and only assign new_cluster additionally so we can do the cleanup? > cluster_offset = start_of_cluster(s, s->free_byte_offset); > - if ((cluster_offset + s->cluster_size) == offset) { > + if ((cluster_offset + s->cluster_size) == new_cluster) { > /* we are lucky: contiguous data */ > offset = s->free_byte_offset; > - qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, > - false, QCOW2_DISCARD_NEVER); > + ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, > + 1, false, QCOW2_DISCARD_NEVER); > + if (ret < 0) { > + qcow2_free_clusters(bs, new_cluster, s->cluster_size, > + QCOW2_DISCARD_NEVER); > + return ret; > + } > s->free_byte_offset += size; > } else { > - s->free_byte_offset = offset; > + s->free_byte_offset = new_cluster; > goto redo; > } > } Kevin