Re: [Qemu-devel] [PATCH v4 11/21] qcow2: Handle failure for potentially large allocations

[Kevin Wolf <kwolf@redhat.com>]

Am 05.06.2014 um 16:52 hat Benoît Canet geschrieben:
> The Thursday 05 Jun 2014 à 15:36:23 (+0200), Kevin Wolf wrote :
> > Some code in the block layer makes potentially huge allocations. Failure
> > is not completely unexpected there, so avoid aborting qemu and handle
> > out-of-memory situations gracefully.
> > 
> > This patch addresses the allocations in the qcow2 block driver.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/qcow2-cache.c    | 13 ++++++++++++-
> >  block/qcow2-cluster.c  | 35 +++++++++++++++++++++++++++--------
> >  block/qcow2-refcount.c | 48 ++++++++++++++++++++++++++++++++++++++----------
> >  block/qcow2-snapshot.c | 22 +++++++++++++++++-----
> >  block/qcow2.c          | 41 +++++++++++++++++++++++++++++++++--------
> >  5 files changed, 127 insertions(+), 32 deletions(-)

> > @@ -1562,7 +1574,10 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
> >      if (!is_active_l1) {
> >          /* inactive L2 tables require a buffer to be stored in when loading
> >           * them from disk */
> > -        l2_table = qemu_blockalign(bs, s->cluster_size);
> > +        l2_table = qemu_try_blockalign(bs, s->cluster_size);
> > +        if (l2_table == NULL) {
> > +            return -ENOMEM;
> > +        }
> >      }
> >  
> >      for (i = 0; i < l1_size; i++) {
> > @@ -1740,7 +1755,11 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs)
> >  
> >      nb_clusters = size_to_clusters(s, bs->file->total_sectors *
> >                                     BDRV_SECTOR_SIZE);
> I think we need asset(nb_clusters >= 1); else it will trigger a spurious -ENOMEM
> on very small files.

To be precise, very small means size 0, because size_to_clusters() rounds
up. An image file with size 0 wouldn't have a qcow2 header, though, so we
wouldn't even have opened it in the first place.

(Also, an assert() never fixes a bug. At best, it changes its symptom to
a crash, except when built with NDEBUG defined.)

> > -    expanded_clusters = g_malloc0((nb_clusters + 7) / 8);
> > +    expanded_clusters = g_try_malloc0((nb_clusters + 7) / 8);
> > +    if (expanded_clusters == NULL) {
> > +        ret = -ENOMEM;
> > +        goto fail;
> > +    }
> >  
> >      ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size,
> >                                       &expanded_clusters, &nb_clusters);

Kevin