qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapsh


From: Pavel Hrdina
Subject: Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions
Date: Thu, 06 Sep 2012 11:07:54 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120828 Thunderbird/15.0

On 08/30/2012 04:47 PM, Luiz Capitulino wrote:
On Wed, 15 Aug 2012 09:41:43 +0200
Pavel Hrdina <address@hidden> wrote:

Signed-off-by: Pavel Hrdina <address@hidden>
---
  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(-)

diff --git a/block.c b/block.c
index 016858b..8bc49b7 100644
--- a/block.c
+++ b/block.c
@@ -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)
-        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);

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.

+        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.

+    } 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,
diff --git a/block.h b/block.h
index 2e2be11..92e782b 100644
--- a/block.h
+++ b/block.h
@@ -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);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4e7c93b..cf86dae 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -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;
  }
diff --git a/block/qcow2.h b/block/qcow2.h
index b4eb654..854bd12 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -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);
diff --git a/block/rbd.c b/block/rbd.c
index 5a0f79f..7bc42f0 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -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;
      }
- 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.

      }
return 0;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index a04ad99..fc51e04 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -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;
  }
diff --git a/block_int.h b/block_int.h
index 4452f6f..b76c943 100644
--- a/block_int.h
+++ b/block_int.h
@@ -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);
diff --git a/qemu-img.c b/qemu-img.c
index b41e670..c798d66 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -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));
diff --git a/savevm.c b/savevm.c
index 0ea10c9..1b67af6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -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));




reply via email to

[Prev in Thread] Current Thread [Next in Thread]