[Qemu-devel] [PATCH 05/11] block: update error reporting for bdrv_snapshot_goto() and related functions

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <address@hidden>
---
 block.c                   | 55 ++++++++++++++++++++++++++---------------------
 block/qcow2-snapshot.c    | 32 +++++++++++++++++++--------
 block/qcow2.h             |  8 +++++--
 block/rbd.c               | 19 +++++++++++-----
 block/sheepdog.c          | 33 ++++++++++++++++------------
 include/block/block.h     |  8 ++++---
 include/block/block_int.h |  8 ++++---
 qemu-img.c                | 20 ++++++++++-------
 savevm.c                  | 21 ++++++++++--------
 9 files changed, 127 insertions(+), 77 deletions(-)

diff --git a/block.c b/block.c
index fb065e6..a760a2f 100644
--- a/block.c
+++ b/block.c
@@ -3365,30 +3365,30 @@ 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 '%s' has no medium", 
bdrv_get_device_name(bs));
+    } 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;
+            error_setg(errp, "failed to open '%s'", bdrv_get_device_name(bs));
         }
-        return ret;
+    } else {
+        error_setg(errp, "snapshots are not supported on device '%s'",
+                           bdrv_get_device_name(bs));
     }
-
-    return -ENOTSUP;
 }
 
 void bdrv_snapshot_delete(BlockDriverState *bs,
@@ -3410,16 +3410,23 @@ void bdrv_snapshot_delete(BlockDriverState *bs,
 }
 
 int bdrv_snapshot_list(BlockDriverState *bs,
-                       QEMUSnapshotInfo **psn_info)
+                       QEMUSnapshotInfo **psn_info,
+                       Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_list)
-        return drv->bdrv_snapshot_list(bs, psn_info);
-    if (bs->file)
-        return bdrv_snapshot_list(bs->file, psn_info);
-    return -ENOTSUP;
+
+    if (!drv) {
+        error_setg(errp, "device '%s' has no medium", 
bdrv_get_device_name(bs));
+        return 0;
+    } else if (drv->bdrv_snapshot_list) {
+        return drv->bdrv_snapshot_list(bs, psn_info, errp);
+    } else if (bs->file) {
+        return bdrv_snapshot_list(bs->file, psn_info, errp);
+    } else {
+        error_setg(errp, "snapshots are not supported on device '%s'",
+                   bdrv_get_device_name(bs));
+        return 0;
+    }
 }
 
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 2ebeadc..4a69b88 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, "qcow2: 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, "qcow2: 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, "fqcow2: ailed to grow L1 table");
         goto fail;
     }
 
@@ -465,18 +469,23 @@ 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,
+                         "qcow2: 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,
+                         "qcow2: failed to update snapshot refcount");
         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, "qcow2: failed to save L1 table");
         goto fail;
     }
 
@@ -502,6 +511,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
*snapshot_id)
     }
 
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "qcow2: failed to sync L1 table");
         goto fail;
     }
 
@@ -514,6 +524,8 @@ 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,
+                         "qcow2: failed to update snapshot refcount");
         goto fail;
     }
 
@@ -523,11 +535,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,
@@ -595,7 +606,9 @@ void qcow2_snapshot_delete(BlockDriverState *bs,
 #endif
 }
 
-int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
+int qcow2_snapshot_list(BlockDriverState *bs,
+                        QEMUSnapshotInfo **psn_tab,
+                        Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QEMUSnapshotInfo *sn_tab, *sn_info;
@@ -604,7 +617,8 @@ int qcow2_snapshot_list(BlockDriverState *bs, 
QEMUSnapshotInfo **psn_tab)
 
     if (!s->nb_snapshots) {
         *psn_tab = NULL;
-        return s->nb_snapshots;
+        error_setg(errp, "qcow2: there is no snapshot available");
+        return 0;
     }
 
     sn_tab = g_malloc0(s->nb_snapshots * sizeof(QEMUSnapshotInfo));
diff --git a/block/qcow2.h b/block/qcow2.h
index dbd332d..ae62953 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -383,11 +383,15 @@ 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);
-int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
+int qcow2_snapshot_list(BlockDriverState *bs,
+                        QEMUSnapshotInfo **psn_tab,
+                        Error **errp);
 int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
diff --git a/block/rbd.c b/block/rbd.c
index c10edbf..9259621 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -886,18 +886,22 @@ 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, "rbd: failed to rollback snapshot");
+    }
 }
 
 static int qemu_rbd_snap_list(BlockDriverState *bs,
-                              QEMUSnapshotInfo **psn_tab)
+                              QEMUSnapshotInfo **psn_tab,
+                              Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     QEMUSnapshotInfo *sn_info, *sn_tab = NULL;
@@ -913,7 +917,12 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
         }
     } while (snap_count == -ERANGE);
 
-    if (snap_count <= 0) {
+    if (snap_count < 0) {
+        error_setg_errno(errp, -snap_count, "rbd: failed to find snapshots");
+        snap_count = 0;
+    }
+
+    if (snap_count == 0) {
         goto done;
     }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 270fa64..3d44575 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1838,7 +1838,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;
@@ -1863,12 +1865,14 @@ 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, "sd: failed to find VDI snapshot '%s'",
+                         vdi);
         goto out;
     }
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
+        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
         ret = fd;
         goto out;
     }
@@ -1880,14 +1884,15 @@ static int sd_snapshot_goto(BlockDriverState *bs, const 
char *snapshot_id)
     closesocket(fd);
 
     if (ret) {
+        error_setg_errno(errp, -ret, "sd: 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, "sd: invalid snapshot '%s'", snapshot_id);
         goto out;
     }
 
@@ -1896,16 +1901,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,
@@ -1916,11 +1917,13 @@ static void sd_snapshot_delete(BlockDriverState *bs,
     return;
 }
 
-static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
+static int sd_snapshot_list(BlockDriverState *bs,
+                            QEMUSnapshotInfo **psn_tab,
+                            Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     SheepdogReq req;
-    int fd, nr = 1024, ret, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
+    int fd, nr = 1024, ret = 0, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
     QEMUSnapshotInfo *sn_tab = NULL;
     unsigned wlen, rlen;
     int found = 0;
@@ -1934,7 +1937,7 @@ static int sd_snapshot_list(BlockDriverState *bs, 
QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
-        ret = fd;
+        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
         goto out;
     }
 
@@ -1950,6 +1953,7 @@ static int sd_snapshot_list(BlockDriverState *bs, 
QEMUSnapshotInfo **psn_tab)
 
     closesocket(fd);
     if (ret) {
+        error_setg_errno(errp, -ret, "sd: failed to read VDIs");
         goto out;
     }
 
@@ -1961,7 +1965,7 @@ static int sd_snapshot_list(BlockDriverState *bs, 
QEMUSnapshotInfo **psn_tab)
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
-        ret = fd;
+        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
         goto out;
     }
 
@@ -2001,7 +2005,8 @@ out:
     g_free(vdi_inuse);
 
     if (ret < 0) {
-        return ret;
+        error_setg_errno(errp, -ret, "sd: failed to read VDI object");
+        return 0;
     }
 
     return found;
diff --git a/include/block/block.h b/include/block/block.h
index db8c6aa..33d498b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,13 +335,15 @@ 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);
 int bdrv_snapshot_list(BlockDriverState *bs,
-                       QEMUSnapshotInfo **psn_info);
+                       QEMUSnapshotInfo **psn_info,
+                       Error **errp);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 53c52bb..c56d923 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -155,13 +155,15 @@ 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);
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
-                              QEMUSnapshotInfo **psn_info);
+                              QEMUSnapshotInfo **psn_info,
+                              Error **errp);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
                                   const char *snapshot_name);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
diff --git a/qemu-img.c b/qemu-img.c
index 4828fe4..06cd8af 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1563,10 +1563,13 @@ static void dump_snapshots(BlockDriverState *bs)
     QEMUSnapshotInfo *sn_tab, *sn;
     int nb_sns, i;
     char buf[256];
+    Error *local_err = NULL;
 
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns <= 0)
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+    if (qemu_img_handle_error("qemu-img dump snapshots failed", local_err)
+            || nb_sns == 0) {
         return;
+    }
     printf("Snapshot list:\n");
     printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
     for(i = 0; i < nb_sns; i++) {
@@ -1598,7 +1601,10 @@ static void collect_snapshots(BlockDriverState *bs , 
ImageInfo *info)
     int i, sn_count;
     QEMUSnapshotInfo *sn_tab = NULL;
     SnapshotInfoList *info_list, *cur_item = NULL;
-    sn_count = bdrv_snapshot_list(bs, &sn_tab);
+    Error *local_err = NULL;
+    sn_count = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+
+    qemu_img_handle_error("qemu-img collect snapshots failed", local_err);
 
     for (i = 0; i < sn_count; i++) {
         info->has_snapshots = true;
@@ -2026,11 +2032,9 @@ 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("qemu-img snapshot apply failed",
+                                    local_err);
         break;
 
     case SNAPSHOT_DELETE:
diff --git a/savevm.c b/savevm.c
index 4af7d2d..ca4a42e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2206,7 +2206,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info,
         return found;
     }
 
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
     if (nb_sns < 0) {
         return found;
     }
@@ -2401,6 +2401,7 @@ int load_vmstate(const char *name)
     QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
+    Error *local_err;
 
     bs_vm_state = bdrv_snapshots();
     if (!bs_vm_state) {
@@ -2445,11 +2446,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)) {
+                error_report("%s", error_get_pretty(local_err));
+                error_free(local_err);
+                return -1;
             }
         }
     }
@@ -2528,6 +2529,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
     int total;
     int *available_snapshots;
     char buf[256];
+    Error *local_err = NULL;
 
     bs = bdrv_snapshots();
     if (!bs) {
@@ -2535,9 +2537,10 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0) {
-        monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
+    if (error_is_set(&local_err)) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
         return;
     }
 
-- 
1.8.1.4