Re: [Qemu-devel] [PATCH v2 02/17] block: add error parameter to bdrv_snapshot_create() and related functions

[Luiz Capitulino]

[Forgot adding block layer guys, re-sending]

On Thu, 13 Dec 2012 16:40:36 +0100
Pavel Hrdina <address@hidden> wrote:

> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
>  block.c                | 26 ++++++++++++++++++--------
>  block.h                |  3 ++-
>  block/qcow2-snapshot.c | 11 ++++++++++-
>  block/qcow2.h          |  4 +++-
>  block/rbd.c            | 17 +++++++++++------
>  block/sheepdog.c       | 18 ++++++++++--------
>  block_int.h            |  3 ++-
>  qemu-img.c             |  8 +++-----
>  savevm.c               |  2 +-
>  9 files changed, 60 insertions(+), 32 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c05875f..fea429b 100644
> --- a/block.c
> +++ b/block.c
> @@ -3075,16 +3075,26 @@ 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_setg(errp, "Device '%s' has no medium.",
> +                   bdrv_get_device_name(bs));
> +        ret = -ENOMEDIUM;
> +    } 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_setg(errp, "Not supported.");
> +        ret = -ENOTSUP;
> +    }
> +
> +    return ret;

This function returns an error code _and_ an error object. This is fine
if you drop the error code in a later patch, but you don't do it in this
series.

IMO, we shouldn't return both unless there's a strong reason to do so
(ie. you need the error code and a very specific error message, or you
plan to drop the error code in the future). If this is the case here,
please tell us.

When converting functions that return an error code, I check if any
caller of the function check for a specific error code. If no caller
does this, then I just drop the error code (but in a different patch).

If a caller actually checks the error code, you generally have two
options:

 1. See if checking the error code is actually required, if it's
    not then just drop it

 2. Don't add the Error object to the function returning the error code.
    Add it to the callers instead

We (block layer guys and me) had a discussion about this some weeks ago,
I don't exactly remember if what I described above matches our conclusions,
so I'm CC'ing them.

>  }
>  
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block.h b/block.h
> index 722c620..6abb687 100644
> --- a/block.h
> +++ b/block.h
> @@ -321,7 +321,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..3fd8aff 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"

I think you want to include error.h. qerror.h is legacy.

>  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,7 @@ 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_setg(errp, "Parameter 'name' has to be unique ID.");

Here you're converting qcow2_snapshot_create() to propagate the error, and
I see that you do it for more functions below.

Basically, this kind of conversion should be split into one per-patch or
even one per-series depending on the complexity.

A good series converting do_foo() to take an Error object would do:

 1. Add an Error object to do_foo() and fill it properly. Callers of do_foo()
    should pass NULL for the new argument

 2. For each caller of do_foo() create a new patch, which changes the caller
    to pass an Error object to do_foo() and use it internally

 3. If do_foo() returns an error code, drop it

>          return -EEXIST;
>      }
>  
> @@ -348,6 +352,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>      l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * 
> sizeof(uint64_t));
>      if (l1_table_offset < 0) {
>          ret = l1_table_offset;
> +        error_setg(errp, "Failed to allocate L1 table.");
>          goto fail;
>      }
>  
> @@ -362,6 +367,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>      ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
>                        s->l1_size * sizeof(uint64_t));
>      if (ret < 0) {
> +        error_setg(errp, "Failed to save L1 table.");
>          goto fail;
>      }
>  
> @@ -375,11 +381,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>       */
>      ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 
> 1);
>      if (ret < 0) {
> +        error_setg(errp, "Failed to update snapshot refcount.");
>          goto fail;
>      }
>  
>      ret = bdrv_flush(bs);
>      if (ret < 0) {
> +        error_setg(errp, "Failed to flush data.");
>          goto fail;
>      }
>  
> @@ -397,6 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>      if (ret < 0) {
>          g_free(s->snapshots);
>          s->snapshots = old_snapshot_list;
> +        error_setg(errp, "Failed to write new snapshots.");
>          goto fail;
>      }
>  
> 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 f3becc7..cb5acf8 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>
>  
> @@ -811,12 +812,14 @@ 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_setg(errp, "Parameter 'name' cannot be empty.");
>          return -EINVAL; /* we need a name for rbd snapshots */
>      }
>  
> @@ -826,17 +829,19 @@ 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_setg(errp, "ID and name have to be equal.");
>          return -EINVAL;
>      }
>  
>      if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> +        error_setg(errp, "Parameter 'name' has to be shorter that 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_setg(errp, "Failed to create snapshot.");
> +        return ret;
>      }
>  
>      return 0;
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index a48f58c..094634a 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
>  
> @@ -1735,7 +1736,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;
> @@ -1748,9 +1751,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_setg(errp, "You can't create a snapshot '%s' of a VDI 
> snapshot.",
> +                   sn_info->name);
>          return -EINVAL;
>      }
>  
> @@ -1769,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>      fd = connect_to_sdog(s->addr, s->port);
>      if (fd < 0) {
>          ret = fd;
> +        error_setg(errp, "Failed to connect to sdog.");
>          goto cleanup;
>      }
>  
>      ret = write_object(fd, (char *)&s->inode, 
> vid_to_vdi_oid(s->inode.vdi_id),
>                         s->inode.nr_copies, datalen, 0, false, 
> s->cache_enabled);
>      if (ret < 0) {
> -        error_report("failed to write snapshot's inode.");
> +        error_setg(errp, "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));
> +        error_setg(errp, "Failed to create inode for snapshot.");
>          goto cleanup;
>      }
>  
> @@ -1793,7 +1795,7 @@ 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));
> +        error_setg(errp, "Failed to read new inode info.");
>          goto cleanup;
>      }
>  
> diff --git a/block_int.h b/block_int.h
> index 9deedb8..976ea1f 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -147,7 +147,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 e29e01b..93e4022 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1486,6 +1486,7 @@ static int img_snapshot(int argc, char **argv)
>      int c, ret = 0, bdrv_oflags;
>      int action = 0;
>      qemu_timeval tv;
> +    Error *err = NULL;
>  
>      bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
>      /* Parse commandline parameters */
> @@ -1559,11 +1560,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);
> -        if (ret) {
> -            error_report("Could not create snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> -        }
> +        ret = bdrv_snapshot_create(bs, &sn, &err);
>          break;
>  
>      case SNAPSHOT_APPLY:
> @@ -1585,6 +1582,7 @@ static int img_snapshot(int argc, char **argv)
>  
>      /* Cleanup */
>      bdrv_delete(bs);
> +    handle_error(&err);
>      if (ret) {
>          return 1;
>      }
> diff --git a/savevm.c b/savevm.c
> index 5d04d59..ef2f305 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2211,7 +2211,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));