From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37369) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5GbR-0003ob-02 for qemu-devel@nongnu.org; Mon, 20 Jan 2014 10:17:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W5GbJ-0008CM-50 for qemu-devel@nongnu.org; Mon, 20 Jan 2014 10:17:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:15650) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5GbI-0008CI-Sm for qemu-devel@nongnu.org; Mon, 20 Jan 2014 10:17:21 -0500 Date: Mon, 20 Jan 2014 16:17:16 +0100 From: Kevin Wolf Message-ID: <20140120151716.GG3267@dhcp-200-207.str.redhat.com> References: <33b97a2f2e4860e69e658143d1e63927fa12a92e.1388381026.git.hutao@cn.fujitsu.com> <52DBFABD.7010907@redhat.com> <20140120030427.GD24882@G08FNSTD100614.fnst.cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140120030427.GD24882@G08FNSTD100614.fnst.cn.fujitsu.com> Subject: Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , Max Reitz Am 20.01.2014 um 04:04 hat Hu Tao geschrieben: > On Sun, Jan 19, 2014 at 05:18:05PM +0100, Max Reitz wrote: > > On 30.12.2013 06:29, Hu Tao wrote: > > >In case of do preallocating metadata with a large cluster size, > > >qcow2_alloc_cluster_offset() can allocate nothing and returns > > >a NULL l2meta. This patch checks for it and link2 l2 with only > > >valid l2meta. > > > > > >Signed-off-by: Hu Tao > > >--- > > > block/qcow2.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > >diff --git a/block/qcow2.c b/block/qcow2.c > > >index 46860d5..380c240 100644 > > >--- a/block/qcow2.c > > >+++ b/block/qcow2.c > > >@@ -1399,18 +1399,20 @@ static int preallocate(BlockDriverState *bs) > > > offset = 0; > > > while (nb_sectors) { > > >- num = MIN(nb_sectors, INT_MAX >> 9); > > >+ num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS); > > > > Well, if you're already adjusting this here, you could also replace > > the other occurrences of 9 and 512 in this function. ;-) > > > > > ret = qcow2_alloc_cluster_offset(bs, offset, &num, > > > &host_offset, &meta); > > > if (ret < 0) { > > > return ret; > > > } > > >- ret = qcow2_alloc_cluster_link_l2(bs, meta); > > >- if (ret < 0) { > > >- qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters, > > >- QCOW2_DISCARD_NEVER); > > >- return ret; > > >+ if (meta) { > > >+ ret = qcow2_alloc_cluster_link_l2(bs, meta); > > >+ if (ret < 0) { > > >+ qcow2_free_any_clusters(bs, meta->alloc_offset, > > >+ meta->nb_clusters, QCOW2_DISCARD_NEVER); > > >+ return ret; > > >+ } > > > } > > > /* There are no dependent requests, but we need to remove our request > > > > But this doesn't make this patch wrong, so: > > > > Reviewed-by: Max Reitz > > Max, > > Thanks for all of you comments! The series looks good in general, but I think the comments are worth addressing before we merge it. I would also love to see an qemu-iotests case that tests the cases that would previously crash. Once you post a new version that addresses these points, I'll merge it. Kevin