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

[Kevin Wolf]

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

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