Re: [Qemu-devel] [PATCH v1 3/3] qcow2: check for NULL l2meta

[Hu Tao]

On Tue, Jan 21, 2014 at 12:04:47PM +0100, Kevin Wolf wrote:
> Am 21.01.2014 um 07:02 hat Hu Tao geschrieben:
> > On Tue, Jan 21, 2014 at 11:33:18AM +0800, Hu Tao wrote:
> > > On Mon, Jan 20, 2014 at 04:17:16PM +0100, Kevin Wolf wrote:
> > > > 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 <address@hidden>
> > > > > > >---
> > > > > > >  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 <address@hidden>
> > > > > 
> > > > > 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.
> > 
> > Should I add the test case into an existing file or create a new file?
> 
> The closest existing case I found is 049, which is however more about
> option parsing rather than the actual effect of the options. I think a
> new file might be better.
> 
> If you add a new case, can you please use 079 as its number? There are
> several yet unmerged patch series in flight that take the lower numbers.

OK.