Patchwork [02/18] block: add error parameter to bdrv_snapshot_create() and related functions

login
register
mail settings
Submitter Pavel Hrdina
Date Aug. 15, 2012, 7:41 a.m.
Message ID <3222678753b33b3a95096767745a5cf313c7ca1b.1345016001.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/177555/
State New
Headers show

Comments

Pavel Hrdina - Aug. 15, 2012, 7:41 a.m.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 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(-)
Luiz Capitulino - Aug. 30, 2012, 2:47 p.m.
On Wed, 15 Aug 2012 09:41:43 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  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);

> +        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));
Markus Armbruster - Aug. 31, 2012, 6:26 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 15 Aug 2012 09:41:43 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>  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);
>
>> +        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.

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

Let's not reinvent errno, poorly.

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

Better.

What's ErrorClass doing there?

[...]


[*] Almost; it's still QEMU after all.
Kevin Wolf - Aug. 31, 2012, 12:18 p.m.
Am 31.08.2012 08:26, schrieb Markus Armbruster:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> On Wed, 15 Aug 2012 09:41:43 +0200
>> Pavel Hrdina <phrdina@redhat.com> wrote:
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>  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);
>>
>>> +        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.

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

Kevin
Luiz Capitulino - Aug. 31, 2012, 1:09 p.m.
On Fri, 31 Aug 2012 08:26:38 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 15 Aug 2012 09:41:43 +0200
> > Pavel Hrdina <phrdina@redhat.com> wrote:
> >
> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >> ---
> >>  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);
> >
> >> +        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.
> 
> >
> >  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.
> 
> Let's not reinvent errno, poorly.
> 
> >        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.
> 
> Better.
> 
> What's ErrorClass doing there?

There might be cases where we will have to report an error other than GenericError,
we could have error_sete_generic(), but this is starting to get weird :)

> 
> [...]
> 
> 
> [*] Almost; it's still QEMU after all.
>
Luiz Capitulino - Aug. 31, 2012, 1:13 p.m.
On Fri, 31 Aug 2012 14:18:55 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 31.08.2012 08:26, schrieb Markus Armbruster:
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> > 
> >> On Wed, 15 Aug 2012 09:41:43 +0200
> >> Pavel Hrdina <phrdina@redhat.com> wrote:
> >>
> >>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >>> ---
> >>>  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);
> >>
> >>> +        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.
> 
> 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).

Yes, I agree. Let me evaluate this calmly. I'll try to come with a better
proposal soon.
Pavel Hrdina - Sept. 6, 2012, 9:07 a.m.
On 08/30/2012 04:47 PM, Luiz Capitulino wrote:
> On Wed, 15 Aug 2012 09:41:43 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   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));
Luiz Capitulino - Sept. 10, 2012, 5:03 p.m.
On Thu, 06 Sep 2012 11:07:54 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:

> On 08/30/2012 04:47 PM, Luiz Capitulino wrote:
> > On Wed, 15 Aug 2012 09:41:43 +0200
> > Pavel Hrdina <phrdina@redhat.com> wrote:
> >
> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >> ---
> >>   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));

Hmm, no. The best way to refactor code that use the same error message
like the one above is to move the whole operation (with error handling
included) to a wrapper and use the wrapper instead.

Adding macros for error messages is a Bad Thing because soon or later
someone will create a new macro for a very similar but different error
message (which will lead to macro explosion) or will use an existing
macro that doesn't fit well to the failure being reported.

> 
> 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));
>

Patch

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));
+        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_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;
     }
 
     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));