Re: [Qemu-devel] [PATCH v5 07/26] qcow2: Respect error in qcow2_alloc_bytes()

[Max Reitz]

On 2015-02-04 at 06:40, Kevin Wolf wrote:
> 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 <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > ---
> >   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.)

Right, will fix.

> > +            }
> > +        }
> >       } 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).

Because @offset is always set. In case the next condition is true, it is 
set to s->free_byte_offset, just like it was before. In case it isn't, 
s->free_byte_offset will be set to @new_cluster and the loop will be 
started again (probably always resulting in size <= free_in_cluster 
being true and thus @offset being set to s->free_byte_offset).

> Why can't we keep offset where it was used and only assign new_cluster
> additionally so we can do the cleanup?

Why should we? If I were a reader of this code, I think it would confuse 
me to have two variables holding the same value but for some reason only 
using @offset, but once it's been overwritten, suddenly using 
@new_cluster (whereas one could just use @new_cluster everywhere, and 
then writing @offset would be superfluous). Also, the idea was having 
@offset hold the offset of where the compressed data is to be stored, 
whereas @new_cluster is just the offset of a new cluster, but not 
necessarily the offset where compressed data will be stored (it won't be 
if (cluster_offset + s->cluster_size) == new_cluster).

Max

> >           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