diff mbox

[v3,07/19] block: expect errors from bdrv_co_is_allocated

Message ID 1374762197-7261-8-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini July 25, 2013, 2:23 p.m. UTC 
Some bdrv_is_allocated callers do not expect errors, but the fallback
in qcow2.c might make other callers trip on assertion failures or
infinite loops.

Fix the callers to always look for errors.

Cc: qemu-stable@nongnu.org
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow2.c  | 4 +---
 block/stream.c | 2 +-
 qemu-img.c     | 5 +++++
 3 files changed, 7 insertions(+), 4 deletions(-)

Comments

Kevin Wolf July 29, 2013, 1:43 p.m. UTC | #1 
Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> Some bdrv_is_allocated callers do not expect errors, but the fallback
> in qcow2.c might make other callers trip on assertion failures or
> infinite loops.
> 
> Fix the callers to always look for errors.
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Good idea, but there seem to be a few more callers that don't handle
errors yet. Not sure what your criterion for fixing callers or not was.
Did you simply miss the rest?

Kevin
Paolo Bonzini July 29, 2013, 2:03 p.m. UTC | #2 
Il 29/07/2013 15:43, Kevin Wolf ha scritto:
> Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
>> Some bdrv_is_allocated callers do not expect errors, but the fallback
>> in qcow2.c might make other callers trip on assertion failures or
>> infinite loops.
>>
>> Fix the callers to always look for errors.
>>
>> Cc: qemu-stable@nongnu.org
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Good idea, but there seem to be a few more callers that don't handle
> errors yet. Not sure what your criterion for fixing callers or not was.
> Did you simply miss the rest?

In general, I didn't bother about callers that didn't handle *pnum = 0
correctly (for example block-migration.c and commit).  But I see now
that the ones in qemu-io-cmds.c are due to a bad rebase.

(BTW, I just noticed now that both block-migration.c and commit also run
outside a coroutine, and thus rely on bdrv_is_allocated creating one).

Paolo
Kevin Wolf July 29, 2013, 2:17 p.m. UTC | #3 
Am 29.07.2013 um 16:03 hat Paolo Bonzini geschrieben:
> Il 29/07/2013 15:43, Kevin Wolf ha scritto:
> > Am 25.07.2013 um 16:23 hat Paolo Bonzini geschrieben:
> >> Some bdrv_is_allocated callers do not expect errors, but the fallback
> >> in qcow2.c might make other callers trip on assertion failures or
> >> infinite loops.
> >>
> >> Fix the callers to always look for errors.
> >>
> >> Cc: qemu-stable@nongnu.org
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Good idea, but there seem to be a few more callers that don't handle
> > errors yet. Not sure what your criterion for fixing callers or not was.
> > Did you simply miss the rest?
> 
> In general, I didn't bother about callers that didn't handle *pnum = 0
> correctly (for example block-migration.c and commit).  But I see now
> that the ones in qemu-io-cmds.c are due to a bad rebase.

Would you mind fixing the already broken ones as well?

> (BTW, I just noticed now that both block-migration.c and commit also run
> outside a coroutine, and thus rely on bdrv_is_allocated creating one).

Ah well, I missed those, too bad. I won't insist on removing the
synchronous path then.

Kevin
diff mbox

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index 0eceefe..e2b4202 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -648,13 +648,11 @@  static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,
     int ret;
 
     *pnum = nb_sectors;
-    /* FIXME We can get errors here, but the bdrv_co_is_allocated interface
-     * can't pass them on today */
     qemu_co_mutex_lock(&s->lock);
     ret = qcow2_get_cluster_offset(bs, sector_num << 9, pnum, &cluster_offset);
     qemu_co_mutex_unlock(&s->lock);
     if (ret < 0) {
-        *pnum = 0;
+        return ret;
     }
 
     return (cluster_offset != 0) || (ret == QCOW2_CLUSTER_ZERO);
diff --git a/block/stream.c b/block/stream.c
index 60136fb..1579cd3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -120,7 +120,7 @@  wait:
         if (ret == 1) {
             /* Allocated in the top, no need to copy.  */
             copy = false;
-        } else {
+        } else if (ret >= 0) {
             /* Copy if allocated in the intermediate images.  Limit to the
              * known-unallocated area [sector_num, sector_num+n).  */
             ret = bdrv_is_allocated_above(bs->backing_hd, base,
diff --git a/qemu-img.c b/qemu-img.c
index c55ca5c..a4957eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2073,6 +2073,11 @@  static int img_rebase(int argc, char **argv)
 
             /* If the cluster is allocated, we don't need to take action */
             ret = bdrv_is_allocated(bs, sector, n, &n);
+            if (ret < 0) {
+                error_report("error while reading image metadata: %s",
+                             strerror(-ret));
+                goto out;
+            }
             if (ret) {
                 continue;
             }