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

[Kevin Wolf]

Am 04.02.2015 um 16:04 hat Max Reitz geschrieben:
> 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).

Yes, I misread. Hooray for goto. One more reason for a cleanup of the
whole function.

Kevin