[Qemu-devel] [PATCH] qcow2: don't ignore failed update_refcount

[Qemu-devel] [PATCH] qcow2: don't ignore failed update_refcount

[Jim Meyering]

update_refcount is marked as a function for which we must use its result,

    static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,

and rightly so, since doing otherwise would amount to ignoring write failure.
However, there are two cases in which the return value is currently ignored.
This fixes them:

>From 107940556a2d0ef1de1d59a5da0c6c3086246817 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Mon, 8 Feb 2010 11:50:59 +0100
Subject: [PATCH] qcow2: don't ignore failed update_refcount

* block/qcow2-refcount.c (grow_refcount_table): When update_refcount
fails, return its negative return code to our caller.
(alloc_refcount_block): Likewise.
---
 block/qcow2-refcount.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 2fdc26b..b9f5093 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -181,7 +181,9 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size)
     s->refcount_table_size = new_table_size;
     s->refcount_table_offset = table_offset;

-    update_refcount(bs, table_offset, new_table_size2, 1);
+    ret = update_refcount(bs, table_offset, new_table_size2, 1);
+    if (ret < 0)
+	goto fail;
     qcow2_free_clusters(bs, old_table_offset, old_table_size * sizeof(uint64_t));
     return 0;
  fail:
@@ -231,7 +233,9 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)

         refcount_block_offset = offset;
         s->refcount_block_cache_offset = offset;
-        update_refcount(bs, offset, s->cluster_size, 1);
+        ret = update_refcount(bs, offset, s->cluster_size, 1);
+        if (ret < 0)
+            return ret;
         cache_refcount_updates = cache;
     } else {
         if (refcount_block_offset != s->refcount_block_cache_offset) {
--
1.7.0.rc2.156.g2ac04

[Qemu-devel] Re: [PATCH] qcow2: don't ignore failed update_refcount

[Kevin Wolf]

Am 08.02.2010 16:01, schrieb Jim Meyering:
> update_refcount is marked as a function for which we must use its result,
> 
>     static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
> 
> and rightly so, since doing otherwise would amount to ignoring write failure.
> However, there are two cases in which the return value is currently ignored.
> This fixes them:
> 
> From 107940556a2d0ef1de1d59a5da0c6c3086246817 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering@redhat.com>
> Date: Mon, 8 Feb 2010 11:50:59 +0100
> Subject: [PATCH] qcow2: don't ignore failed update_refcount
> 
> * block/qcow2-refcount.c (grow_refcount_table): When update_refcount
> fails, return its negative return code to our caller.
> (alloc_refcount_block): Likewise.

I'm currently working on fixing exactly this, and unfortunaly, no, it's
not that easy. What you introduce looks like proper error handling at
first sight, but what happens in fact is that while the current write
request correctly fails now we're running with corrupted metadata for
all future requests (the new refcount table/block is already in use, but
it has a refcount of 0 and will be overwritten sooner or later).

Actually, I have found it impossible to fix the current approach, so the
fix I'm working on will be more of a rewrite of the two functions.

Kevin

[Qemu-devel] Re: [PATCH] qcow2: don't ignore failed update_refcount

[Jim Meyering]

Kevin Wolf wrote:
...
> I'm currently working on fixing exactly this, and unfortunaly, no, it's
> not that easy. What you introduce looks like proper error handling at
> first sight, but what happens in fact is that while the current write
> request correctly fails now we're running with corrupted metadata for
> all future requests (the new refcount table/block is already in use, but
> it has a refcount of 0 and will be overwritten sooner or later).
>
> Actually, I have found it impossible to fix the current approach, so the
> fix I'm working on will be more of a rewrite of the two functions.

Nicely worded NACK ;-)
Thanks.