qapi: Convert savevm, loadvm, delvm and info snapshots

Signed-off-by Pavel Hrdina <[email protected]>

---
 qerror.h | 3 +++
 1 file changed, 3 insertions(+)

qerror.h +0 -0

index d0a76a4..7e0bae7 100644

qerror.h +0 -0

 #define QERR_INVALID_BLOCK_FORMAT \
     ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
 

Quoted 13 lines · On Wed, 15 Aug 2012 09:41:42 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
  qerror.h | 3 +++
  1 file changed, 3 insertions(+)

qerror.h +0 -0

index d0a76a4..7e0bae7 100644
@@ -120,6 +120,9 @@ void assert_no_error(Error *err);
  #define QERR_FEATURE_DISABLED \
      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
 

qerror.h void assert_no_error(Error *err); · line 125

Quoted 3 lines

qerror.h +3 -0

+#define QERR_GENERIC_ERROR \
+    ERROR_CLASS_GENERIC_ERROR, "An (Errno %d) error has occurred"
+

You should use error_setg() instead:

http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04980.html

There usage examples in the series introducing it. It would be better to wait for it to be merged before you use it though, as it's always possible for people to ask for changes.

Quoted 3 lines

qerror.h +0 -0

  #define QERR_INVALID_BLOCK_FORMAT \
      ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
 

Signed-off-by Pavel Hrdina <[email protected]>

---
 block.c                | 25 +++++++++++++++++--------
 block.h                |  3 ++-
 block/qcow2-snapshot.c |  9 ++++++++-
 block/qcow2.h          |  4 +++-
 block/rbd.c            | 20 ++++++++++++++------
 block/sheepdog.c       | 17 +++++++++--------
 block_int.h            |  3 ++-
 qemu-img.c             |  2 +-
 savevm.c               |  2 +-
 9 files changed, 57 insertions(+), 28 deletions(-)

block.c +2 -2

index 016858b..8bc49b7 100644
@@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
 }
 
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)

block.c +10 -0

+    } else if (drv->bdrv_snapshot_create) {
+        ret = drv->bdrv_snapshot_create(bs, sn_info, errp);
+    } else if (bs->file) {
+        ret = bdrv_snapshot_create(bs->file, sn_info, errp);
+    } else {
+        error_set(errp, QERR_NOT_SUPPORTED);
+        ret = -ENOTSUP;
+    }
+
+    return ret;
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,

block.h +2 -1

index 2e2be11..92e782b 100644
@@ -296,7 +296,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info);
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp);
 int bdrv_snapshot_goto(BlockDriverState *bs,
                        const char *snapshot_id);
 int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);

block/qcow2-snapshot.c +8 -1

index 4e7c93b..cf86dae 100644
@@ -25,6 +25,7 @@
 #include "qemu-common.h"
 #include "block_int.h"
 #include "block/qcow2.h"
+#include "qerror.h"
 
 typedef struct QEMU_PACKED QCowSnapshotHeader {
     /* header is 8 byte aligned */

@@ -312,7 +313,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
 }
 
 /* if no id is provided, a new one is constructed */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+int qcow2_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *new_snapshot_list = NULL;

@@ -331,6 +334,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
     /* Check that the ID is unique */
     if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "non-existing id identifier");
         return -EEXIST;
     }
 

@@ -415,6 +420,8 @@ fail:
     g_free(sn->name);
     g_free(l1_table);
 
+    error_set(errp, QERR_GENERIC_ERROR, ret);
+
     return ret;
 }
 

block/qcow2.h +3 -1

index b4eb654..854bd12 100644
@@ -308,7 +308,9 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
 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_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp);
 int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
 int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);

block/rbd.c +10 -2

index 5a0f79f..7bc42f0 100644
@@ -16,6 +16,7 @@
 #include "qemu-common.h"
 #include "qemu-error.h"
 #include "block_int.h"
+#include "qerror.h"
 
 #include <rbd/librbd.h>
 

@@ -817,12 +818,15 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
 }
 
 static int qemu_rbd_snap_create(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info)
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
-    int r;
+    int ret;
 
     if (sn_info->name[0] == '\0') {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "some tag identifier");
         return -EINVAL; /* we need a name for rbd snapshots */
     }
 

@@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
      */
     if (sn_info->id_str[0] != '\0' &&
         strcmp(sn_info->id_str, sn_info->name) != 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "id and tag to equal");
         return -EINVAL;
     }
 
     if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "shorter than 127 chars");
         return -ERANGE;

block/rbd.c +0 -0

     }
 
     return 0;

block/sheepdog.c +9 -8

index a04ad99..fc51e04 100644
@@ -17,6 +17,7 @@
 #include "qemu_socket.h"
 #include "block_int.h"
 #include "bitops.h"
+#include "qerror.h"
 
 #define SD_PROTO_VER 0x01
 

@@ -1730,7 +1731,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
     return 0;
 }
 
-static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+static int sd_snapshot_create(BlockDriverState *bs,
+                              QEMUSnapshotInfo *sn_info,
+                              Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     int ret, fd;

@@ -1743,9 +1746,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
             s->name, sn_info->vm_state_size, s->is_snapshot);
 
     if (s->is_snapshot) {
-        error_report("You can't create a snapshot of a snapshot VDI, "
-                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
-
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", " not VDI snapshot");
         return -EINVAL;
     }
 

@@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
                        s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
     if (ret < 0) {
-        error_report("failed to write snapshot's inode.");
         goto cleanup;
     }
 
     ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
                        s->addr, s->port);
     if (ret < 0) {
-        error_report("failed to create inode for snapshot. %s",
-                     strerror(errno));
         goto cleanup;
     }
 

@@ -1785,7 +1784,6 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
                       s->inode.nr_copies, datalen, 0, s->cache_enabled);
 
     if (ret < 0) {
-        error_report("failed to read new inode info. %s", strerror(errno));
         goto cleanup;
     }
 

@@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
 cleanup:
     closesocket(fd);
+    if (ret < 0) {
+        error_set(errp, QERR_GENERIC_ERROR, ret);
+    }
     return ret;
 }
 

block_int.h +2 -1

index 4452f6f..b76c943 100644
@@ -206,7 +206,8 @@ struct BlockDriver {
                                  const uint8_t *buf, int nb_sectors);
 
     int (*bdrv_snapshot_create)(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info);
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                               const char *snapshot_id);
     int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);

qemu-img.c +1 -1

index b41e670..c798d66 100644
@@ -1267,7 +1267,7 @@ static int img_snapshot(int argc, char **argv)
         sn.date_sec = tv.tv_sec;
         sn.date_nsec = tv.tv_usec * 1000;
 
-        ret = bdrv_snapshot_create(bs, &sn);
+        ret = bdrv_snapshot_create(bs, &sn, NULL);
         if (ret) {
             error_report("Could not create snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));

savevm.c +1 -1

index 0ea10c9..1b67af6 100644
@@ -2165,7 +2165,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         if (bdrv_can_snapshot(bs1)) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
+            ret = bdrv_snapshot_create(bs1, sn, NULL);
             if (ret < 0) {
                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                                bdrv_get_device_name(bs1));

Quoted 27 lines · On Wed, 15 Aug 2012 09:41:43 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
 block.c                | 25 +++++++++++++++++--------
 block.h                |  3 ++-
 block/qcow2-snapshot.c |  9 ++++++++-
 block/qcow2.h          |  4 +++-
 block/rbd.c            | 20 ++++++++++++++------
 block/sheepdog.c       | 17 +++++++++--------
 block_int.h            |  3 ++-
 qemu-img.c             |  2 +-
 savevm.c               |  2 +-
 9 files changed, 57 insertions(+), 28 deletions(-)

block.c +2 -2

index 016858b..8bc49b7 100644
@@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
 }
 
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)

block.c BlockDriverState *bdrv_snapshots(void) · line 2671

Quoted 10 lines

block.c +4 -6

-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
-    return -ENOTSUP;
+    int ret;
+
+    if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));

We should only use QERR_ macros for the errors listed in the ErrorClass enum (except GenericError), all other errors should generally use error_setg(), like this:

error_setg(errp, "device '%s' has no medium);

block.c BlockDriverState *bdrv_snapshots(void) · line 2672

Quoted 1 line

block.c +1 -0

+        ret = -ENOMEDIUM;

And, usually, we should get rid of errno propagation. There are two cases here:

1. errno is propagated up so that upper layers can print a decent error message to the user.

In this case, it's safe to eliminate errno. error_setg() will store a decent message already and the Error object can be propagated up.

2. errno is propagated up so that upper layers can distinguish among error causes and take different actions accordingly.

Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only used to communicate the error to the user). However, I'm pretty sure that such usage exists in qemu and the error API will break it, as most of our errors are generic.

I see two solutions to this problem:

A. Add specific errors to ErrorClass. I don't like this very much,
as it's possible that such errors are going to be useful only internally.

B. Add two new functions:

void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...); int error_get_errno(const Error **err);

So that we can maintain errno when it's used to communicate error cause among functions.

Quoted 47 lines · Luiz Capitulino

Quoted 27 lines · On Wed, 15 Aug 2012 09:41:43 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
 block.c                | 25 +++++++++++++++++--------
 block.h                |  3 ++-
 block/qcow2-snapshot.c |  9 ++++++++-
 block/qcow2.h          |  4 +++-
 block/rbd.c            | 20 ++++++++++++++------
 block/sheepdog.c       | 17 +++++++++--------
 block_int.h            |  3 ++-
 qemu-img.c             |  2 +-
 savevm.c               |  2 +-
 9 files changed, 57 insertions(+), 28 deletions(-)

block.c +2 -2

index 016858b..8bc49b7 100644
@@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
 }
 
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)

block.c BlockDriverState *bdrv_snapshots(void) · line 2671

Quoted 10 lines

block.c +4 -6

-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
-    return -ENOTSUP;
+    int ret;
+
+    if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));

We should only use QERR_ macros for the errors listed in the ErrorClass enum (except GenericError), all other errors should generally use error_setg(), like this:

error_setg(errp, "device '%s' has no medium);

Quoted 3 lines

block.c BlockDriverState *bdrv_snapshots(void) · line 2672

Quoted 1 line

block.c +1 -0

+        ret = -ENOMEDIUM;

And, usually, we should get rid of errno propagation. There are two cases here:

The block layer consistently[*] uses -errno return values. Its consistency is valuable, and I'm a bit reluctant to break it. Maybe a new rule "returns -errno, except when it has an Error ** argument" could work. I'd like to hear Kevin's advice on this.

Am 31.08.2012 08:26, schrieb Markus Armbruster:

Quoted 53 lines

Quoted 50 lines · Luiz Capitulino

Quoted 27 lines · On Wed, 15 Aug 2012 09:41:43 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
 block.c                | 25 +++++++++++++++++--------
 block.h                |  3 ++-
 block/qcow2-snapshot.c |  9 ++++++++-
 block/qcow2.h          |  4 +++-
 block/rbd.c            | 20 ++++++++++++++------
 block/sheepdog.c       | 17 +++++++++--------
 block_int.h            |  3 ++-
 qemu-img.c             |  2 +-
 savevm.c               |  2 +-
 9 files changed, 57 insertions(+), 28 deletions(-)

block.c +2 -2

index 016858b..8bc49b7 100644
@@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
 }
 
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)

block.c BlockDriverState *bdrv_snapshots(void) · line 2671

Quoted 10 lines

block.c +4 -6

-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
-    return -ENOTSUP;
+    int ret;
+
+    if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));

We should only use QERR_ macros for the errors listed in the ErrorClass enum (except GenericError), all other errors should generally use error_setg(), like this:

error_setg(errp, "device '%s' has no medium);

block.c BlockDriverState *bdrv_snapshots(void) · line 2672

Quoted 1 line

block.c +1 -0

+        ret = -ENOMEDIUM;

And, usually, we should get rid of errno propagation. There are two cases here:

Quoted 4 lines

The block layer consistently[*] uses -errno return values. Its consistency is valuable, and I'm a bit reluctant to break it. Maybe a new rule "returns -errno, except when it has an Error ** argument" could work. I'd like to hear Kevin's advice on this.

I'd rather not remove existing -errno returns if there's not good reason why we can't provide them (or can't do so easily at least).

Quoted 45 lines · Luiz Capitulino

Quoted 27 lines · On Wed, 15 Aug 2012 09:41:43 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
 block.c                | 25 +++++++++++++++++--------
 block.h                |  3 ++-
 block/qcow2-snapshot.c |  9 ++++++++-
 block/qcow2.h          |  4 +++-
 block/rbd.c            | 20 ++++++++++++++------
 block/sheepdog.c       | 17 +++++++++--------
 block_int.h            |  3 ++-
 qemu-img.c             |  2 +-
 savevm.c               |  2 +-
 9 files changed, 57 insertions(+), 28 deletions(-)

block.c +2 -2

index 016858b..8bc49b7 100644
@@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
 }
 
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)

block.c BlockDriverState *bdrv_snapshots(void) · line 2671

Quoted 10 lines

block.c +4 -6

-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
-    return -ENOTSUP;
+    int ret;
+
+    if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));

We should only use QERR_ macros for the errors listed in the ErrorClass enum (except GenericError), all other errors should generally use error_setg(), like this:

Quoted 5 lines

error_setg(errp, "device '%s' has no medium);

block.c BlockDriverState *bdrv_snapshots(void) · line 2672

Quoted 1 line

block.c +1 -0

+        ret = -ENOMEDIUM;

And, usually, we should get rid of errno propagation. There are two cases here:

The block layer consistently[*] uses -errno return values. Its consistency is valuable, and I'm a bit reluctant to break it. Maybe a new rule "returns -errno, except when it has an Error ** argument" could work. I'd like to hear Kevin's advice on this.

Quoted 15 lines

1. errno is propagated up so that upper layers can print a decent error message to the user.

In this case, it's safe to eliminate errno. error_setg() will store a decent message already and the Error object can be propagated up.

2. errno is propagated up so that upper layers can distinguish among error causes and take different actions accordingly.

Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only used to communicate the error to the user). However, I'm pretty sure that such usage exists in qemu and the error API will break it, as most of our errors are generic.

Quoted 4 lines

I see two solutions to this problem:

A. Add specific errors to ErrorClass. I don't like this very much,
as it's possible that such errors are going to be useful only internally.

Let's not reinvent errno, poorly.

Quoted 5 lines

       B. Add two new functions:

            void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...);
            int error_get_errno(const Error **err);

Quoted 2 lines

So that we can maintain errno when it's used to communicate error cause among functions.

Better.

What's ErrorClass doing there?

[...]

[*] Almost; it's still QEMU after all.

Quoted 27 lines · On Wed, 15 Aug 2012 09:41:43 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
  block.c                | 25 +++++++++++++++++--------
  block.h                |  3 ++-
  block/qcow2-snapshot.c |  9 ++++++++-
  block/qcow2.h          |  4 +++-
  block/rbd.c            | 20 ++++++++++++++------
  block/sheepdog.c       | 17 +++++++++--------
  block_int.h            |  3 ++-
  qemu-img.c             |  2 +-
  savevm.c               |  2 +-
  9 files changed, 57 insertions(+), 28 deletions(-)

block.c +2 -2

index 016858b..8bc49b7 100644
@@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
  }
 
  int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
  {
      BlockDriver *drv = bs->drv;
-    if (!drv)

block.c BlockDriverState *bdrv_snapshots(void) · line 2671

Quoted 10 lines

block.c +4 -6

-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
-    return -ENOTSUP;
+    int ret;
+
+    if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));

We should only use QERR_ macros for the errors listed in the ErrorClass enum (except GenericError), all other errors should generally use error_setg(), like this:

  error_setg(errp, "device '%s' has no medium);

block.c BlockDriverState *bdrv_snapshots(void) · line 2672

Quoted 2 lines

block.c +1 -0

+        ret = -ENOMEDIUM;

And, usually, we should get rid of errno propagation. There are two cases here:

1. errno is propagated up so that upper layers can print a decent error message to the user.

In this case, it's safe to eliminate errno. error_setg() will store a decent message already and the Error object can be propagated up.

2. errno is propagated up so that upper layers can distinguish among error causes and take different actions accordingly.

Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only used to communicate the error to the user). However, I'm pretty sure that such usage exists in qemu and the error API will break it, as most of our errors are generic.

I see two solutions to this problem:

A. Add specific errors to ErrorClass. I don't like this very much,
as it's possible that such errors are going to be useful only internally.

B. Add two new functions:

void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...); int error_get_errno(const Error **err);

So that we can maintain errno when it's used to communicate error cause among functions.

Quoted 126 lines

block.c +10 -0

+    } else if (drv->bdrv_snapshot_create) {
+        ret = drv->bdrv_snapshot_create(bs, sn_info, errp);
+    } else if (bs->file) {
+        ret = bdrv_snapshot_create(bs->file, sn_info, errp);
+    } else {
+        error_set(errp, QERR_NOT_SUPPORTED);
+        ret = -ENOTSUP;
+    }
+
+    return ret;
  }
 
  int bdrv_snapshot_goto(BlockDriverState *bs,

block.h +2 -1

index 2e2be11..92e782b 100644
@@ -296,7 +296,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
  int bdrv_is_snapshot(BlockDriverState *bs);
  BlockDriverState *bdrv_snapshots(void);
  int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info);
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp);
  int bdrv_snapshot_goto(BlockDriverState *bs,
                         const char *snapshot_id);
  int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);

block/qcow2-snapshot.c +8 -1

index 4e7c93b..cf86dae 100644
@@ -25,6 +25,7 @@
  #include "qemu-common.h"
  #include "block_int.h"
  #include "block/qcow2.h"
+#include "qerror.h"
 
  typedef struct QEMU_PACKED QCowSnapshotHeader {
      /* header is 8 byte aligned */

@@ -312,7 +313,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
  }
 
  /* if no id is provided, a new one is constructed */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+int qcow2_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp)
  {
      BDRVQcowState *s = bs->opaque;
      QCowSnapshot *new_snapshot_list = NULL;

@@ -331,6 +334,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
      /* Check that the ID is unique */
      if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "non-existing id identifier");
          return -EEXIST;
      }
 

@@ -415,6 +420,8 @@ fail:
      g_free(sn->name);
      g_free(l1_table);
 
+    error_set(errp, QERR_GENERIC_ERROR, ret);
+
      return ret;
  }
 

block/qcow2.h +3 -1

index b4eb654..854bd12 100644
@@ -308,7 +308,9 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
  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_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp);
  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
  int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
  int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);

block/rbd.c +10 -2

index 5a0f79f..7bc42f0 100644
@@ -16,6 +16,7 @@
  #include "qemu-common.h"
  #include "qemu-error.h"
  #include "block_int.h"
+#include "qerror.h"
 
  #include <rbd/librbd.h>
 

@@ -817,12 +818,15 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
  }
 
  static int qemu_rbd_snap_create(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info)
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp)
  {
      BDRVRBDState *s = bs->opaque;
-    int r;
+    int ret;
 
      if (sn_info->name[0] == '\0') {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "some tag identifier");
          return -EINVAL; /* we need a name for rbd snapshots */
      }
 

@@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
       */
      if (sn_info->id_str[0] != '\0' &&
          strcmp(sn_info->id_str, sn_info->name) != 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "id and tag to equal");
          return -EINVAL;
      }
 
      if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "shorter than 127 chars");
          return -ERANGE;

block/rbd.c static int qemu_rbd_snap_create(BlockDriverState *bs, · line 853

Quoted 10 lines

block/rbd.c +4 -4

      }
 
-    r = rbd_snap_create(s->image, sn_info->name);
-    if (r < 0) {
-        error_report("failed to create snap: %s", strerror(-r));
-        return r;
+    ret = rbd_snap_create(s->image, sn_info->name);
+    if (ret < 0) {
+        error_set(errp, QERR_GENERIC_ERROR, ret);
+        return ret;

You should always be careful when dropping error_report(), most of the time you also have to change callers. In this case, bdrv_snapshot_create() is called by qemu-img, which should be changed to print the error from the Error object.

Quoted 112 lines

block/rbd.c +0 -0

      }
 
      return 0;

block/sheepdog.c +9 -8

index a04ad99..fc51e04 100644
@@ -17,6 +17,7 @@
  #include "qemu_socket.h"
  #include "block_int.h"
  #include "bitops.h"
+#include "qerror.h"
 
  #define SD_PROTO_VER 0x01
 

@@ -1730,7 +1731,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
      return 0;
  }
 
-static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+static int sd_snapshot_create(BlockDriverState *bs,
+                              QEMUSnapshotInfo *sn_info,
+                              Error **errp)
  {
      BDRVSheepdogState *s = bs->opaque;
      int ret, fd;

@@ -1743,9 +1746,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
              s->name, sn_info->vm_state_size, s->is_snapshot);
 
      if (s->is_snapshot) {
-        error_report("You can't create a snapshot of a snapshot VDI, "
-                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
-
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", " not VDI snapshot");
          return -EINVAL;
      }
 

@@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
                         s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
      if (ret < 0) {
-        error_report("failed to write snapshot's inode.");
          goto cleanup;
      }
 
      ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
                         s->addr, s->port);
      if (ret < 0) {
-        error_report("failed to create inode for snapshot. %s",
-                     strerror(errno));
          goto cleanup;
      }
 

@@ -1785,7 +1784,6 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
                        s->inode.nr_copies, datalen, 0, s->cache_enabled);
 
      if (ret < 0) {
-        error_report("failed to read new inode info. %s", strerror(errno));
          goto cleanup;
      }
 

@@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
  cleanup:
      closesocket(fd);
+    if (ret < 0) {
+        error_set(errp, QERR_GENERIC_ERROR, ret);
+    }
      return ret;
  }
 

block_int.h +2 -1

index 4452f6f..b76c943 100644
@@ -206,7 +206,8 @@ struct BlockDriver {
                                   const uint8_t *buf, int nb_sectors);
 
      int (*bdrv_snapshot_create)(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info);
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp);
      int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                                const char *snapshot_id);
      int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);

qemu-img.c +1 -1

index b41e670..c798d66 100644
@@ -1267,7 +1267,7 @@ static int img_snapshot(int argc, char **argv)
          sn.date_sec = tv.tv_sec;
          sn.date_nsec = tv.tv_usec * 1000;
 
-        ret = bdrv_snapshot_create(bs, &sn);
+        ret = bdrv_snapshot_create(bs, &sn, NULL);
          if (ret) {
              error_report("Could not create snapshot '%s': %d (%s)",
                  snapshot_name, ret, strerror(-ret));

savevm.c +1 -1

index 0ea10c9..1b67af6 100644
@@ -2165,7 +2165,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
          if (bdrv_can_snapshot(bs1)) {
              /* Write VM state size only to the image that contains the state */
              sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
+            ret = bdrv_snapshot_create(bs1, sn, NULL);
              if (ret < 0) {
                  monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                                 bdrv_get_device_name(bs1));

Quoted 44 lines · Luiz Capitulino

Quoted 27 lines · On Wed, 15 Aug 2012 09:41:43 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
  block.c                | 25 +++++++++++++++++--------
  block.h                |  3 ++-
  block/qcow2-snapshot.c |  9 ++++++++-
  block/qcow2.h          |  4 +++-
  block/rbd.c            | 20 ++++++++++++++------
  block/sheepdog.c       | 17 +++++++++--------
  block_int.h            |  3 ++-
  qemu-img.c             |  2 +-
  savevm.c               |  2 +-
  9 files changed, 57 insertions(+), 28 deletions(-)

block.c +2 -2

index 016858b..8bc49b7 100644
@@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void)
  }
 
  int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
  {
      BlockDriver *drv = bs->drv;
-    if (!drv)

block.c BlockDriverState *bdrv_snapshots(void) · line 2671

Quoted 10 lines

block.c +4 -6

-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
-    return -ENOTSUP;
+    int ret;
+
+    if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));

We should only use QERR_ macros for the errors listed in the ErrorClass enum (except GenericError), all other errors should generally use error_setg(), like this:

Quoted 1 line

  error_setg(errp, "device '%s' has no medium);

I agree that we should use error_setg() for all GenericError, but I think that would be better have macros for errors which are used many-times . For example:

#define QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM "Device '%s' has no medium"

and use error_setg(errp, QERR_ERRMSG_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));

I think that the consistency of error messages is important and an error message should be the same for every error occurrence.

Quoted 283 lines

block.c BlockDriverState *bdrv_snapshots(void) · line 2672

Quoted 2 lines

block.c +1 -0

+        ret = -ENOMEDIUM;

And, usually, we should get rid of errno propagation. There are two cases here:

1. errno is propagated up so that upper layers can print a decent error message to the user.

In this case, it's safe to eliminate errno. error_setg() will store a decent message already and the Error object can be propagated up.

2. errno is propagated up so that upper layers can distinguish among error causes and take different actions accordingly.

Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only used to communicate the error to the user). However, I'm pretty sure that such usage exists in qemu and the error API will break it, as most of our errors are generic.

I see two solutions to this problem:

A. Add specific errors to ErrorClass. I don't like this very much,
as it's possible that such errors are going to be useful only internally.

B. Add two new functions:

void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...); int error_get_errno(const Error **err);

So that we can maintain errno when it's used to communicate error cause among functions.

Quoted 126 lines

block.c +10 -0

+    } else if (drv->bdrv_snapshot_create) {
+        ret = drv->bdrv_snapshot_create(bs, sn_info, errp);
+    } else if (bs->file) {
+        ret = bdrv_snapshot_create(bs->file, sn_info, errp);
+    } else {
+        error_set(errp, QERR_NOT_SUPPORTED);
+        ret = -ENOTSUP;
+    }
+
+    return ret;
  }
 
  int bdrv_snapshot_goto(BlockDriverState *bs,

block.h +2 -1

index 2e2be11..92e782b 100644
@@ -296,7 +296,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
  int bdrv_is_snapshot(BlockDriverState *bs);
  BlockDriverState *bdrv_snapshots(void);
  int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info);
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp);
  int bdrv_snapshot_goto(BlockDriverState *bs,
                         const char *snapshot_id);
  int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);

block/qcow2-snapshot.c +8 -1

index 4e7c93b..cf86dae 100644
@@ -25,6 +25,7 @@
  #include "qemu-common.h"
  #include "block_int.h"
  #include "block/qcow2.h"
+#include "qerror.h"
 
  typedef struct QEMU_PACKED QCowSnapshotHeader {
      /* header is 8 byte aligned */

@@ -312,7 +313,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
  }
 
  /* if no id is provided, a new one is constructed */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+int qcow2_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp)
  {
      BDRVQcowState *s = bs->opaque;
      QCowSnapshot *new_snapshot_list = NULL;

@@ -331,6 +334,8 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
      /* Check that the ID is unique */
      if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "non-existing id identifier");
          return -EEXIST;
      }
 

@@ -415,6 +420,8 @@ fail:
      g_free(sn->name);
      g_free(l1_table);
 
+    error_set(errp, QERR_GENERIC_ERROR, ret);
+
      return ret;
  }
 

block/qcow2.h +3 -1

index b4eb654..854bd12 100644
@@ -308,7 +308,9 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
  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_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp);
  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
  int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
  int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);

block/rbd.c +10 -2

index 5a0f79f..7bc42f0 100644
@@ -16,6 +16,7 @@
  #include "qemu-common.h"
  #include "qemu-error.h"
  #include "block_int.h"
+#include "qerror.h"
 
  #include <rbd/librbd.h>
 

@@ -817,12 +818,15 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
  }
 
  static int qemu_rbd_snap_create(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info)
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp)
  {
      BDRVRBDState *s = bs->opaque;
-    int r;
+    int ret;
 
      if (sn_info->name[0] == '\0') {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "some tag identifier");
          return -EINVAL; /* we need a name for rbd snapshots */
      }
 

@@ -832,17 +836,21 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
       */
      if (sn_info->id_str[0] != '\0' &&
          strcmp(sn_info->id_str, sn_info->name) != 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "id and tag to equal");
          return -EINVAL;
      }
 
      if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "shorter than 127 chars");
          return -ERANGE;

block/rbd.c static int qemu_rbd_snap_create(BlockDriverState *bs, · line 853

Quoted 10 lines

block/rbd.c +4 -4

      }
 
-    r = rbd_snap_create(s->image, sn_info->name);
-    if (r < 0) {
-        error_report("failed to create snap: %s", strerror(-r));
-        return r;
+    ret = rbd_snap_create(s->image, sn_info->name);
+    if (ret < 0) {
+        error_set(errp, QERR_GENERIC_ERROR, ret);
+        return ret;

You should always be careful when dropping error_report(), most of the time you also have to change callers. In this case, bdrv_snapshot_create() is called by qemu-img, which should be changed to print the error from the Error object.

Quoted 112 lines

block/rbd.c +0 -0

      }
 
      return 0;

block/sheepdog.c +9 -8

index a04ad99..fc51e04 100644
@@ -17,6 +17,7 @@
  #include "qemu_socket.h"
  #include "block_int.h"
  #include "bitops.h"
+#include "qerror.h"
 
  #define SD_PROTO_VER 0x01
 

@@ -1730,7 +1731,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
      return 0;
  }
 
-static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+static int sd_snapshot_create(BlockDriverState *bs,
+                              QEMUSnapshotInfo *sn_info,
+                              Error **errp)
  {
      BDRVSheepdogState *s = bs->opaque;
      int ret, fd;

@@ -1743,9 +1746,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
              s->name, sn_info->vm_state_size, s->is_snapshot);
 
      if (s->is_snapshot) {
-        error_report("You can't create a snapshot of a snapshot VDI, "
-                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
-
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", " not VDI snapshot");
          return -EINVAL;
      }
 

@@ -1767,15 +1769,12 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
                         s->inode.nr_copies, datalen, 0, 0, s->cache_enabled);
      if (ret < 0) {
-        error_report("failed to write snapshot's inode.");
          goto cleanup;
      }
 
      ret = do_sd_create(s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
                         s->addr, s->port);
      if (ret < 0) {
-        error_report("failed to create inode for snapshot. %s",
-                     strerror(errno));
          goto cleanup;
      }
 

@@ -1785,7 +1784,6 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
                        s->inode.nr_copies, datalen, 0, s->cache_enabled);
 
      if (ret < 0) {
-        error_report("failed to read new inode info. %s", strerror(errno));
          goto cleanup;
      }
 

@@ -1795,6 +1793,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
  cleanup:
      closesocket(fd);
+    if (ret < 0) {
+        error_set(errp, QERR_GENERIC_ERROR, ret);
+    }
      return ret;
  }
 

block_int.h +2 -1

index 4452f6f..b76c943 100644
@@ -206,7 +206,8 @@ struct BlockDriver {
                                   const uint8_t *buf, int nb_sectors);
 
      int (*bdrv_snapshot_create)(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info);
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp);
      int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                                const char *snapshot_id);
      int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);

qemu-img.c +1 -1

index b41e670..c798d66 100644
@@ -1267,7 +1267,7 @@ static int img_snapshot(int argc, char **argv)
          sn.date_sec = tv.tv_sec;
          sn.date_nsec = tv.tv_usec * 1000;
 
-        ret = bdrv_snapshot_create(bs, &sn);
+        ret = bdrv_snapshot_create(bs, &sn, NULL);
          if (ret) {
              error_report("Could not create snapshot '%s': %d (%s)",
                  snapshot_name, ret, strerror(-ret));

savevm.c +1 -1

index 0ea10c9..1b67af6 100644
@@ -2165,7 +2165,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
          if (bdrv_can_snapshot(bs1)) {
              /* Write VM state size only to the image that contains the state */
              sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
+            ret = bdrv_snapshot_create(bs1, sn, NULL);
              if (ret < 0) {
                  monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                                 bdrv_get_device_name(bs1));

Signed-off-by Pavel Hrdina <[email protected]>

---
 block.c                | 26 +++++++++++++++-----------
 block.h                |  3 ++-
 block/qcow2-snapshot.c | 11 ++++++++---
 block/qcow2.h          |  4 +++-
 block/rbd.c            |  6 +++++-
 block/sheepdog.c       | 16 +++++++++-------
 block_int.h            |  3 ++-
 qemu-img.c             |  2 +-
 savevm.c               |  2 +-
 9 files changed, 46 insertions(+), 27 deletions(-)

block.c +2 -1

index 8bc49b7..ad25184 100644
@@ -2683,29 +2683,33 @@ int bdrv_snapshot_create(BlockDriverState *bs,
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id)
+                       const char *snapshot_id,
+                       Error **errp)
 {
     BlockDriver *drv = bs->drv;
     int ret, open_ret;

block.c +10 -4

+    } else if (drv->bdrv_snapshot_goto) {
+        ret = drv->bdrv_snapshot_goto(bs, snapshot_id, errp);
+    } else if (bs->file) {
         drv->bdrv_close(bs);
-        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
+        ret = bdrv_snapshot_goto(bs->file, snapshot_id, errp);
         open_ret = drv->bdrv_open(bs, bs->open_flags);
         if (open_ret < 0) {
             bdrv_delete(bs->file);
             bs->drv = NULL;
-            return open_ret;
+            error_set(errp, QERR_OPEN_FILE_FAILED, bdrv_get_device_name(bs));
+            ret = open_ret;
         }
-        return ret;
+    } else {
+        error_set(errp, QERR_NOT_SUPPORTED);
+        ret = -ENOTSUP;
     }
 
-    return -ENOTSUP;
+    return ret;
 }
 
 int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)

block.h +2 -1

index 92e782b..11edcd3 100644
@@ -299,7 +299,8 @@ int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info,
                          Error **errp);
 int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id);
+                       const char *snapshot_id,
+                       Error **errp);
 int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);

block/qcow2-snapshot.c +8 -3

index cf86dae..8a87b0c 100644
@@ -426,7 +426,9 @@ fail:
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_goto(BlockDriverState *bs,
+                        const char *snapshot_id,
+                        Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;

@@ -438,13 +440,13 @@ 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) {
+        error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_id);
         return -ENOENT;
     }
     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");
+        error_set(errp, QERR_NOT_SUPPORTED);
         ret = -ENOTSUP;
         goto fail;
     }

@@ -536,6 +538,9 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
 
 fail:
     g_free(sn_l1_table);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_GENERIC_ERROR, ret);
+    }
     return ret;
 }
 

block/qcow2.h +3 -1

index 854bd12..6babb56 100644
@@ -311,7 +311,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 int qcow2_snapshot_create(BlockDriverState *bs,
                           QEMUSnapshotInfo *sn_info,
                           Error **errp);
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
+int qcow2_snapshot_goto(BlockDriverState *bs,
+                        const char *snapshot_id,
+                        Error **errp);
 int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
 int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);

block/rbd.c +0 -1

index 7bc42f0..5a73d8a 100644
@@ -867,12 +867,16 @@ static int qemu_rbd_snap_remove(BlockDriverState *bs,
 }
 
 static int qemu_rbd_snap_rollback(BlockDriverState *bs,
-                                  const char *snapshot_name)

block/rbd.c +0 -0

     return r;
 }
 

block/sheepdog.c +9 -7

index fc51e04..557ff92 100644
@@ -1799,7 +1799,9 @@ cleanup:
     return ret;
 }
 
-static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_goto(BlockDriverState *bs,
+                            const char *snapshot_id,
+                            Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     BDRVSheepdogState *old_s;

@@ -1824,13 +1826,12 @@ 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_set(errp, QERR_OPEN_FILE_FAILED, vdi);
         goto out;
     }
 
     fd = connect_to_sdog(s->addr, s->port);
     if (fd < 0) {
-        error_report("failed to connect");
         ret = fd;
         goto out;
     }

@@ -1848,7 +1849,8 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     memcpy(&s->inode, buf, sizeof(s->inode));
 
     if (!s->inode.vm_state_size) {
-        error_report("Invalid snapshot");
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "name", "existing snapshot");
         ret = -ENOENT;
         goto out;
     }

@@ -1864,9 +1866,9 @@ out:
     memcpy(s, old_s, sizeof(BDRVSheepdogState));
     g_free(buf);
     g_free(old_s);
-
-    error_report("failed to open. recover old bdrv_sd_state.");
-
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_GENERIC_ERROR, ret);
+    }
     return ret;
 }
 

block_int.h +2 -1

index b76c943..fa6d72f 100644
@@ -209,7 +209,8 @@ struct BlockDriver {
                                 QEMUSnapshotInfo *sn_info,
                                 Error **errp);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
-                              const char *snapshot_id);
+                              const char *snapshot_id,
+                              Error **errp);
     int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);

qemu-img.c +1 -1

index c798d66..1f10ff4 100644
@@ -1275,7 +1275,7 @@ static int img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_APPLY:
-        ret = bdrv_snapshot_goto(bs, snapshot_name);
+        ret = bdrv_snapshot_goto(bs, snapshot_name, NULL);
         if (ret) {
             error_report("Could not apply snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));

savevm.c +1 -1

index 1b67af6..0931e54 100644
@@ -2256,7 +2256,7 @@ int load_vmstate(const char *name)
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs)) {
-            ret = bdrv_snapshot_goto(bs, name);
+            ret = bdrv_snapshot_goto(bs, name, NULL);
             if (ret < 0) {
                 error_report("Error %d while activating snapshot '%s' on '%s'",
                              ret, name, bdrv_get_device_name(bs));

Signed-off-by Pavel Hrdina <[email protected]>

---
 migration.c |  2 +-
 savevm.c    | 44 ++++++++++++++++++++++++++++----------------
 sysemu.h    |  3 ++-
 3 files changed, 31 insertions(+), 18 deletions(-)

migration.c +1 -1

index ec2f267..f048faf 100644
@@ -88,7 +88,7 @@ int qemu_start_incoming_migration(const char *uri, Error **errp)
 
 void process_incoming_migration(QEMUFile *f)
 {
-    if (qemu_loadvm_state(f) < 0) {
+    if (qemu_loadvm_state(f, NULL) < 0) {
         fprintf(stderr, "load of migration failed\n");
         exit(0);
     }

savevm.c +19 -11

index 0d54115..500eb72 100644
@@ -1916,7 +1916,8 @@ typedef struct LoadStateEntry {
     int version_id;
 } LoadStateEntry;
 
-int qemu_loadvm_state(QEMUFile *f)
+int qemu_loadvm_state(QEMUFile *f,
+                      Error **errp)
 {
     QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
         QLIST_HEAD_INITIALIZER(loadvm_handlers);

@@ -1925,21 +1926,26 @@ int qemu_loadvm_state(QEMUFile *f)
     unsigned int v;
     int ret;
 
-    if (qemu_savevm_state_blocked(NULL)) {
-        return -EINVAL;
+    if (qemu_savevm_state_blocked(errp)) {
+        return -ENOTSUP;
     }
 
     v = qemu_get_be32(f);
-    if (v != QEMU_VM_FILE_MAGIC)
+    if (v != QEMU_VM_FILE_MAGIC) {
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                  "Unknown vm-state file magic");
         return -EINVAL;
+    }
 
     v = qemu_get_be32(f);
     if (v == QEMU_VM_FILE_VERSION_COMPAT) {
-        fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
+        error_set(errp, QERR_NOT_SUPPORTED);
         return -ENOTSUP;
     }
-    if (v != QEMU_VM_FILE_VERSION)
+    if (v != QEMU_VM_FILE_VERSION) {
+        error_set(errp, QERR_NOT_SUPPORTED);
         return -ENOTSUP;
+    }
 
     while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
         uint32_t instance_id, version_id, section_id;

@@ -1961,15 +1967,18 @@ int qemu_loadvm_state(QEMUFile *f)
             /* Find savevm section */
             se = find_se(idstr, instance_id);
             if (se == NULL) {
-                fprintf(stderr, "Unknown savevm section or instance '%s' %d\n", idstr, instance_id);
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "Unknown savevm section or instance '%s' %d",
+                          idstr, instance_id);
                 ret = -EINVAL;
                 goto out;
             }
 
             /* Validate version */
             if (version_id > se->version_id) {
-                fprintf(stderr, "savevm: unsupported version %d for '%s' v%d\n",
-                        version_id, idstr, se->version_id);
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "savevm: unsupported version %d for '%s' v%d",
+                          version_id, idstr, se->version_id);
                 ret = -EINVAL;
                 goto out;
             }

@@ -1984,8 +1993,7 @@ int qemu_loadvm_state(QEMUFile *f)
 
             ret = vmstate_load(f, le->se, le->version_id);
             if (ret < 0) {
-                fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
-                        instance_id, idstr);
+                error_set(errp, QERR_GENERIC_ERROR, ret);
                 goto out;
             }
             break;

savevm.c +7 -4

                 ret = -EINVAL;
                 goto out;
             }
 
             ret = vmstate_load(f, le->se, le->version_id);
             if (ret < 0) {
-                fprintf(stderr, "qemu: warning: error while loading state section id %d\n",
-                        section_id);
+                error_set(errp, QERR_GENERIC_ERROR, ret);
                 goto out;
             }
             break;
         default:
-            fprintf(stderr, "Unknown savevm section type %d\n", section_type);
+            error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                      "Unknown savevm section type %d", section_type);
             ret = -EINVAL;
             goto out;
         }

@@ -2030,6 +2039,9 @@ out:
 
     if (ret == 0) {
         ret = qemu_file_get_error(f);
+        if (ret < 0) {
+            error_set(errp, QERR_GENERIC_ERROR, ret);
+        }
     }
 
     return ret;

@@ -2297,7 +2309,7 @@ int load_vmstate(const char *name)
     }
 
     qemu_system_reset(VMRESET_SILENT);
-    ret = qemu_loadvm_state(f);
+    ret = qemu_loadvm_state(f, NULL);
 
     qemu_fclose(f);
     if (ret < 0) {

sysemu.h +2 -1

index 35d6d67..651d96e 100644
@@ -85,7 +85,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,
 int qemu_savevm_state_complete(QEMUFile *f,
                                Error **errp);
 void qemu_savevm_state_cancel(QEMUFile *f);
-int qemu_loadvm_state(QEMUFile *f);
+int qemu_loadvm_state(QEMUFile *f,
+                      Error **errp);
 
 /* SLIRP */
 void do_info_slirp(Monitor *mon);

Quoted 99 lines · On Wed, 15 Aug 2012 09:41:53 +0200 Pavel Hrdina <[email protected]> wrote:

Signed-off-by Pavel Hrdina <[email protected]>

---
  migration.c |  2 +-
  savevm.c    | 44 ++++++++++++++++++++++++++++----------------
  sysemu.h    |  3 ++-
  3 files changed, 31 insertions(+), 18 deletions(-)

migration.c +1 -1

index ec2f267..f048faf 100644
@@ -88,7 +88,7 @@ int qemu_start_incoming_migration(const char *uri, Error **errp)
 
  void process_incoming_migration(QEMUFile *f)
  {
-    if (qemu_loadvm_state(f) < 0) {
+    if (qemu_loadvm_state(f, NULL) < 0) {
          fprintf(stderr, "load of migration failed\n");
          exit(0);
      }

savevm.c +19 -11

index 0d54115..500eb72 100644
@@ -1916,7 +1916,8 @@ typedef struct LoadStateEntry {
      int version_id;
  } LoadStateEntry;
 
-int qemu_loadvm_state(QEMUFile *f)
+int qemu_loadvm_state(QEMUFile *f,
+                      Error **errp)
  {
      QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
          QLIST_HEAD_INITIALIZER(loadvm_handlers);

@@ -1925,21 +1926,26 @@ int qemu_loadvm_state(QEMUFile *f)
      unsigned int v;
      int ret;
 
-    if (qemu_savevm_state_blocked(NULL)) {
-        return -EINVAL;
+    if (qemu_savevm_state_blocked(errp)) {
+        return -ENOTSUP;
      }
 
      v = qemu_get_be32(f);
-    if (v != QEMU_VM_FILE_MAGIC)
+    if (v != QEMU_VM_FILE_MAGIC) {
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                  "Unknown vm-state file magic");
          return -EINVAL;
+    }
 
      v = qemu_get_be32(f);
      if (v == QEMU_VM_FILE_VERSION_COMPAT) {
-        fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
+        error_set(errp, QERR_NOT_SUPPORTED);
          return -ENOTSUP;
      }
-    if (v != QEMU_VM_FILE_VERSION)
+    if (v != QEMU_VM_FILE_VERSION) {
+        error_set(errp, QERR_NOT_SUPPORTED);
          return -ENOTSUP;
+    }
 
      while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
          uint32_t instance_id, version_id, section_id;

@@ -1961,15 +1967,18 @@ int qemu_loadvm_state(QEMUFile *f)
              /* Find savevm section */
              se = find_se(idstr, instance_id);
              if (se == NULL) {
-                fprintf(stderr, "Unknown savevm section or instance '%s' %d\n", idstr, instance_id);
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "Unknown savevm section or instance '%s' %d",
+                          idstr, instance_id);
                  ret = -EINVAL;
                  goto out;
              }
 
              /* Validate version */
              if (version_id > se->version_id) {
-                fprintf(stderr, "savevm: unsupported version %d for '%s' v%d\n",
-                        version_id, idstr, se->version_id);
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "savevm: unsupported version %d for '%s' v%d",
+                          version_id, idstr, se->version_id);
                  ret = -EINVAL;
                  goto out;
              }

@@ -1984,8 +1993,7 @@ int qemu_loadvm_state(QEMUFile *f)
 
              ret = vmstate_load(f, le->se, le->version_id);
              if (ret < 0) {
-                fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
-                        instance_id, idstr);
+                error_set(errp, QERR_GENERIC_ERROR, ret);
                  goto out;
              }
              break;

@@ -1999,20 +2007,21 @@ int qemu_loadvm_state(QEMUFile *f)

savevm.c int qemu_loadvm_state(QEMUFile *f) · line 2011

Quoted 6 lines

savevm.c +2 -1

                  }
              }
              if (le == NULL) {
-                fprintf(stderr, "Unknown savevm section %d\n", section_id);
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "Unknown savevm section %d", section_id);

You sure that this error message will be printed to the terminal? This has to be done by the caller.

Quoted 53 lines

savevm.c +7 -4

                  ret = -EINVAL;
                  goto out;
              }
 
              ret = vmstate_load(f, le->se, le->version_id);
              if (ret < 0) {
-                fprintf(stderr, "qemu: warning: error while loading state section id %d\n",
-                        section_id);
+                error_set(errp, QERR_GENERIC_ERROR, ret);
                  goto out;
              }
              break;
          default:
-            fprintf(stderr, "Unknown savevm section type %d\n", section_type);
+            error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                      "Unknown savevm section type %d", section_type);
              ret = -EINVAL;
              goto out;
          }

@@ -2030,6 +2039,9 @@ out:
 
      if (ret == 0) {
          ret = qemu_file_get_error(f);
+        if (ret < 0) {
+            error_set(errp, QERR_GENERIC_ERROR, ret);
+        }
      }
 
      return ret;

@@ -2297,7 +2309,7 @@ int load_vmstate(const char *name)
      }
 
      qemu_system_reset(VMRESET_SILENT);
-    ret = qemu_loadvm_state(f);
+    ret = qemu_loadvm_state(f, NULL);
 
      qemu_fclose(f);
      if (ret < 0) {

sysemu.h +2 -1

index 35d6d67..651d96e 100644
@@ -85,7 +85,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,
  int qemu_savevm_state_complete(QEMUFile *f,
                                 Error **errp);
  void qemu_savevm_state_cancel(QEMUFile *f);
-int qemu_loadvm_state(QEMUFile *f);
+int qemu_loadvm_state(QEMUFile *f,
+                      Error **errp);
 
  /* SLIRP */
  void do_info_slirp(Monitor *mon);