diff mbox

[v2,05/12] block: update error reporting for bdrv_snapshot_goto() and related functions

Message ID 057bb7e62d93ed0442c1fbf058c90c53e9931f49.1366817130.git.phrdina@redhat.com
State New
Headers show

Commit Message

Pavel Hrdina April 24, 2013, 3:32 p.m. UTC 
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c                   | 34 ++++++++++++++++++----------------
 block/qcow2-snapshot.c    | 24 +++++++++++++++++-------
 block/qcow2.h             |  4 +++-
 block/rbd.c               | 10 +++++++---
 block/sheepdog.c          | 18 ++++++++----------
 include/block/block.h     |  5 +++--
 include/block/block_int.h |  5 +++--
 qemu-img.c                |  7 ++-----
 savevm.c                  | 10 +++++-----
 9 files changed, 66 insertions(+), 51 deletions(-)

Comments

Eric Blake April 25, 2013, 5:06 p.m. UTC | #1 
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block.c                   | 34 ++++++++++++++++++----------------
>  block/qcow2-snapshot.c    | 24 +++++++++++++++++-------
>  block/qcow2.h             |  4 +++-
>  block/rbd.c               | 10 +++++++---
>  block/sheepdog.c          | 18 ++++++++----------
>  include/block/block.h     |  5 +++--
>  include/block/block_int.h |  5 +++--
>  qemu-img.c                |  7 ++-----
>  savevm.c                  | 10 +++++-----
>  9 files changed, 66 insertions(+), 51 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf May 3, 2013, 11:03 a.m. UTC | #2 
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1867,7 +1867,9 @@ cleanup:
>      return ret;
>  }
>  
> -static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_goto(BlockDriverState *bs,
> +                             const char *snapshot_id,
> +                             Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      BDRVSheepdogState *old_s;
> @@ -1892,13 +1894,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>  
>      ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
>      if (ret) {
> -        error_report("Failed to find_vdi_name");
> +        error_setg_errno(errp, -ret, "Failed to find VDI snapshot '%s'", vdi);

Isn't snapid what identifies the snapshot? Or maybe the combination of
vdi and snapid.

>          goto out;
>      }
>  
>      fd = connect_to_sdog(s);
>      if (fd < 0) {
> -        ret = fd;
> +        error_setg_errno(errp, -fd, "Failed to connect to sdog");
>          goto out;
>      }
>  
> @@ -1909,14 +1911,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>      closesocket(fd);
>  
>      if (ret) {
> +        error_setg_errno(errp, -ret, "Failed to open VDI snapshot '%s'", vdi);
>          goto out;
>      }
>  
>      memcpy(&s->inode, buf, sizeof(s->inode));
>  
>      if (!s->inode.vm_state_size) {
> -        error_report("Invalid snapshot");
> -        ret = -ENOENT;
> +        error_setg(errp, "Invalid snapshot '%s'", snapshot_id);

Here as well.

>          goto out;
>      }
>  
> @@ -1925,16 +1927,12 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>      g_free(buf);
>      g_free(old_s);
>  
> -    return 0;
> +    return;
>  out:
>      /* recover bdrv_sd_state */
>      memcpy(s, old_s, sizeof(BDRVSheepdogState));
>      g_free(buf);
>      g_free(old_s);
> -
> -    error_report("failed to open. recover old bdrv_sd_state.");
> -
> -    return ret;
>  }
>  
>  static void sd_snapshot_delete(BlockDriverState *bs,

> --- a/savevm.c
> +++ b/savevm.c
> @@ -2529,11 +2529,11 @@ int load_vmstate(const char *name)
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs)) {
> -            ret = bdrv_snapshot_goto(bs, name);
> -            if (ret < 0) {
> -                error_report("Error %d while activating snapshot '%s' on '%s'",
> -                             ret, name, bdrv_get_device_name(bs));
> -                return ret;
> +            bdrv_snapshot_goto(bs, name, &local_err);
> +            if (error_is_set(&local_err)) {
> +                qerror_report_err(local_err);

Shouldn't we add the device name on which the failure occurred?

> +                error_free(local_err);
> +                return -EIO;
>              }
>          }
>      }

Kevin
diff mbox

Patch

diff --git a/block.c b/block.c
index 3f7ee38..c36d3d5 100644
--- a/block.c
+++ b/block.c
@@ -3412,30 +3412,32 @@  int bdrv_snapshot_create(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
-int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id)
+void bdrv_snapshot_goto(BlockDriverState *bs,
+                        const char *snapshot_id,
+                        Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    int ret, open_ret;
-
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_goto)
-        return drv->bdrv_snapshot_goto(bs, snapshot_id);
+    int ret;
 
-    if (bs->file) {
+    if (!drv) {
+        error_setg(errp, "Device has no medium");
+    } else if (drv->bdrv_snapshot_goto) {
+        drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
+    } else if (bs->file) {
         drv->bdrv_close(bs);
-        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
-        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
-        if (open_ret < 0) {
+        bdrv_snapshot_goto(bs->file, snapshot_id, errp);
+        ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+        if (ret < 0) {
             bdrv_delete(bs->file);
             bs->drv = NULL;
-            return open_ret;
+            if (!error_is_set(errp)) {
+                error_setg_errno(errp, -ret, "Failed to open '%s'",
+                                 bdrv_get_device_name(bs));
+            }
         }
-        return ret;
+    } else {
+        error_setg(errp, "Snapshots are not supported");
     }
-
-    return -ENOTSUP;
 }
 
 void bdrv_snapshot_delete(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index fd0e231..67a57fc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -417,7 +417,9 @@  fail:
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+void qcow2_snapshot_goto(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;
@@ -429,14 +431,15 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     /* Search the snapshot */
     snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
     if (snapshot_index < 0) {
-        return -ENOENT;
+        error_setg(errp, "Failed to find snapshot '%s' by id or name",
+                   snapshot_id);
+        return;
     }
     sn = &s->snapshots[snapshot_index];
 
     if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
-        error_report("qcow2: Loading snapshots with different disk "
-            "size is not implemented");
-        ret = -ENOTSUP;
+        error_setg(errp, "Loading snapshots with different disk size is not "
+                   "implemented");
         goto fail;
     }
 
@@ -447,6 +450,7 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
      */
     ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to grow L1 table");
         goto fail;
     }
 
@@ -465,18 +469,22 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
 
     ret = bdrv_pread(bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to load data into L1 table");
         goto fail;
     }
 
     ret = qcow2_update_snapshot_refcount(bs, sn->l1_table_offset,
                                          sn->l1_size, 1);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to increase the cluster refcounts "
+                         "of the snapshot to load");
         goto fail;
     }
 
     ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
                            cur_l1_bytes);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to save L1 table");
         goto fail;
     }
 
@@ -502,6 +510,8 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     }
 
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to decrease the cluster refcounts "
+                         "of the old snapshot");
         goto fail;
     }
 
@@ -514,6 +524,7 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
      */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to update the cluster flags");
         goto fail;
     }
 
@@ -523,11 +534,10 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
         qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
-    return 0;
+    return;
 
 fail:
     g_free(sn_l1_table);
-    return ret;
 }
 
 void qcow2_snapshot_delete(BlockDriverState *bs,
diff --git a/block/qcow2.h b/block/qcow2.h
index dbd332d..b70387c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -383,7 +383,9 @@  int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 
 /* qcow2-snapshot.c functions */
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
+void qcow2_snapshot_goto(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         Error **errp);
 void qcow2_snapshot_delete(BlockDriverState *bs,
                            const char *snapshot_id,
                            Error **errp);
diff --git a/block/rbd.c b/block/rbd.c
index 1e54c55..5047b59 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -913,14 +913,18 @@  static void qemu_rbd_snap_remove(BlockDriverState *bs,
     }
 }
 
-static int qemu_rbd_snap_rollback(BlockDriverState *bs,
-                                  const char *snapshot_name)
+static void qemu_rbd_snap_rollback(BlockDriverState *bs,
+                                   const char *snapshot_name,
+                                   Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
 
     r = rbd_snap_rollback(s->image, snapshot_name);
-    return r;
+    if (r < 0) {
+        error_setg_errno(errp, -r, "Failed to rollback snapshot '%s'",
+                         snapshot_name);
+    }
 }
 
 static int qemu_rbd_snap_list(BlockDriverState *bs,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 7e0610f..868ab89 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1867,7 +1867,9 @@  cleanup:
     return ret;
 }
 
-static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+static void sd_snapshot_goto(BlockDriverState *bs,
+                             const char *snapshot_id,
+                             Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     BDRVSheepdogState *old_s;
@@ -1892,13 +1894,13 @@  static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
 
     ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
     if (ret) {
-        error_report("Failed to find_vdi_name");
+        error_setg_errno(errp, -ret, "Failed to find VDI snapshot '%s'", vdi);
         goto out;
     }
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
-        ret = fd;
+        error_setg_errno(errp, -fd, "Failed to connect to sdog");
         goto out;
     }
 
@@ -1909,14 +1911,14 @@  static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     closesocket(fd);
 
     if (ret) {
+        error_setg_errno(errp, -ret, "Failed to open VDI snapshot '%s'", vdi);
         goto out;
     }
 
     memcpy(&s->inode, buf, sizeof(s->inode));
 
     if (!s->inode.vm_state_size) {
-        error_report("Invalid snapshot");
-        ret = -ENOENT;
+        error_setg(errp, "Invalid snapshot '%s'", snapshot_id);
         goto out;
     }
 
@@ -1925,16 +1927,12 @@  static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     g_free(buf);
     g_free(old_s);
 
-    return 0;
+    return;
 out:
     /* recover bdrv_sd_state */
     memcpy(s, old_s, sizeof(BDRVSheepdogState));
     g_free(buf);
     g_free(old_s);
-
-    error_report("failed to open. recover old bdrv_sd_state.");
-
-    return ret;
 }
 
 static void sd_snapshot_delete(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index abb636d..3dccecc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,8 +335,9 @@  int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
-int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id);
+void bdrv_snapshot_goto(BlockDriverState *bs,
+                        const char *snapshot_id,
+                        Error **errp);
 void bdrv_snapshot_delete(BlockDriverState *bs,
                           const char *snapshot_id,
                           Error **errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c3f67c3..db67841 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -154,8 +154,9 @@  struct BlockDriver {
 
     int (*bdrv_snapshot_create)(BlockDriverState *bs,
                                 QEMUSnapshotInfo *sn_info);
-    int (*bdrv_snapshot_goto)(BlockDriverState *bs,
-                              const char *snapshot_id);
+    void (*bdrv_snapshot_goto)(BlockDriverState *bs,
+                               const char *snapshot_id,
+                               Error **errp);
     void (*bdrv_snapshot_delete)(BlockDriverState *bs,
                                  const char *snapshot_id,
                                  Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index 9cf3467..4aca51f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2022,11 +2022,8 @@  static int img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_APPLY:
-        ret = bdrv_snapshot_goto(bs, snapshot_name);
-        if (ret) {
-            error_report("Could not apply snapshot '%s': %d (%s)",
-                snapshot_name, ret, strerror(-ret));
-        }
+        bdrv_snapshot_goto(bs, snapshot_name, &local_err);
+        ret = qemu_img_handle_error(local_err);
         break;
 
     case SNAPSHOT_DELETE:
diff --git a/savevm.c b/savevm.c
index 1b4aea8..f280aae 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2529,11 +2529,11 @@  int load_vmstate(const char *name)
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs)) {
-            ret = bdrv_snapshot_goto(bs, name);
-            if (ret < 0) {
-                error_report("Error %d while activating snapshot '%s' on '%s'",
-                             ret, name, bdrv_get_device_name(bs));
-                return ret;
+            bdrv_snapshot_goto(bs, name, &local_err);
+            if (error_is_set(&local_err)) {
+                qerror_report_err(local_err);
+                error_free(local_err);
+                return -EIO;
             }
         }
     }