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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7939 bytes --]

On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block.c                   | 55 ++++++++++++++++++++++++++---------------------
>  block/qcow2-snapshot.c    | 32 +++++++++++++++++++--------
>  block/qcow2.h             |  8 +++++--
>  block/rbd.c               | 19 +++++++++++-----
>  block/sheepdog.c          | 33 ++++++++++++++++------------
>  include/block/block.h     |  8 ++++---
>  include/block/block_int.h |  8 ++++---
>  qemu-img.c                | 20 ++++++++++-------
>  savevm.c                  | 21 ++++++++++--------
>  9 files changed, 127 insertions(+), 77 deletions(-)
> 

> +    } else if (bs->file) {
>          drv->bdrv_close(bs);
> -        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
> -        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
> -        if (open_ret < 0) {
> +        bdrv_snapshot_goto(bs->file, snapshot_id, errp);
> +        ret = drv->bdrv_open(bs, NULL, bs->open_flags);
> +        if (ret < 0) {
>              bdrv_delete(bs->file);
>              bs->drv = NULL;
> -            return open_ret;
> +            error_setg(errp, "failed to open '%s'", bdrv_get_device_name(bs));

Do we still want to try bdrv_open() if bdrv_snapshot_goto() set errp?
For that matter, if bdrv_snapshot_goto() _did_ set errp, does
error_setg() allow you to overwrite errors, or are you violating
constraints?

This is another case of partial failure; I'm wondering if we need to
eventually tidy this code up to use transaction semantics, so that a
loadvm is all-or-none, rather than risking partial failure.

> @@ -3410,16 +3410,23 @@ void bdrv_snapshot_delete(BlockDriverState *bs,
>  }
>  
>  int bdrv_snapshot_list(BlockDriverState *bs,
> -                       QEMUSnapshotInfo **psn_info)
> +                       QEMUSnapshotInfo **psn_info,
> +                       Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (drv->bdrv_snapshot_list)
> -        return drv->bdrv_snapshot_list(bs, psn_info);
> -    if (bs->file)
> -        return bdrv_snapshot_list(bs->file, psn_info);
> -    return -ENOTSUP;
> +
> +    if (!drv) {
> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
> +        return 0;
> +    } else if (drv->bdrv_snapshot_list) {
> +        return drv->bdrv_snapshot_list(bs, psn_info, errp);
> +    } else if (bs->file) {
> +        return bdrv_snapshot_list(bs->file, psn_info, errp);
> +    } else {
> +        error_setg(errp, "snapshots are not supported on device '%s'",
> +                   bdrv_get_device_name(bs));
> +        return 0;

Why 0 for error?  Don't you want to reserve 0 for 'snapshots are
supported, but none exist', and use -1 for error instead?  Or are we
safe treating no medium and no support in the same was as supported but
the list is 0-length?

> @@ -447,6 +450,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
>       */
>      ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret, "fqcow2: ailed to grow L1 table");

s/ailed/failed/

> @@ -595,7 +606,9 @@ void qcow2_snapshot_delete(BlockDriverState *bs,
>  #endif
>  }
>  
> -int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> +int qcow2_snapshot_list(BlockDriverState *bs,
> +                        QEMUSnapshotInfo **psn_tab,
> +                        Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QEMUSnapshotInfo *sn_tab, *sn_info;
> @@ -604,7 +617,8 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      if (!s->nb_snapshots) {
>          *psn_tab = NULL;
> -        return s->nb_snapshots;
> +        error_setg(errp, "qcow2: there is no snapshot available");
> +        return 0;

Note that inside the 'if' block, s->nb_snapshots == 0, so there is no
change in the return value, but now you are declaring this an error
where it previously just left a NULL *psn_tab.  Does the caller care?

> @@ -913,7 +917,12 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
>          }
>      } while (snap_count == -ERANGE);
>  
> -    if (snap_count <= 0) {
> +    if (snap_count < 0) {
> +        error_setg_errno(errp, -snap_count, "rbd: failed to find snapshots");
> +        snap_count = 0;

This is changing the return value from negative to 0.  Does the caller care?

> +static int sd_snapshot_list(BlockDriverState *bs,
> +                            QEMUSnapshotInfo **psn_tab,
> +                            Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      SheepdogReq req;
> -    int fd, nr = 1024, ret, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
> +    int fd, nr = 1024, ret = 0, max = BITS_TO_LONGS(SD_NR_VDIS) * sizeof(long);
>      QEMUSnapshotInfo *sn_tab = NULL;
>      unsigned wlen, rlen;
>      int found = 0;
> @@ -1934,7 +1937,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>  
>      fd = connect_to_sdog(s);
>      if (fd < 0) {
> -        ret = fd;
> +        error_setg_errno(errp, -fd, "sd: failed to connect to sdog");
>          goto out;

Another case of changing the result from -1 to 0; does the caller care?

> @@ -2001,7 +2005,8 @@ out:
>      g_free(vdi_inuse);
>  
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret, "sd: failed to read VDI object");
> +        return 0;

and again, in the same function.

> +++ b/include/block/block_int.h
> @@ -155,13 +155,15 @@ struct BlockDriver {
>  
>      int (*bdrv_snapshot_create)(BlockDriverState *bs,
>                                  QEMUSnapshotInfo *sn_info);
> -    int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> -                              const char *snapshot_id);
> +    void (*bdrv_snapshot_goto)(BlockDriverState *bs,
> +                               const char *snapshot_id,
> +                               Error **errp);

It would be really nice if we documented the contracts of all these
callback functions.

> +++ b/qemu-img.c
> @@ -1563,10 +1563,13 @@ static void dump_snapshots(BlockDriverState *bs)
>      QEMUSnapshotInfo *sn_tab, *sn;
>      int nb_sns, i;
>      char buf[256];
> +    Error *local_err = NULL;
>  
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns <= 0)
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab, &local_err);
> +    if (qemu_img_handle_error("qemu-img dump snapshots failed", local_err)
> +            || nb_sns == 0) {

Interesting - you are stating that if no error was reported in
local_err, then a return of 0 short-circuits.  I was asking about all
the places where you changed return semantics; if this is the only
caller, then when local_err is set you ignore nb_sns and therefore the
return value is irrelevant (and changing from -1 to 0 makes no
difference to this code doing a short-circuit).  But again, calling that
out as a contract, where we can verify that each implementation of the
callback function obeys the contract, would make me feel a bit better.

> +++ b/savevm.c
> @@ -2206,7 +2206,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>          return found;
>      }
>  
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab, NULL);
>      if (nb_sns < 0) {
>          return found;

Oops.  qemu-img wasn't the only caller.  Here, changing -1 to 0 on error
return means we fall through instead of returning 0 early.  Did you mean
for that to happen?  Calling with NULL says you don't care about error
reporting - is that right?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]