On 04/16/2013 10:05 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrd...@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
signature.asc
Description: OpenPGP digital signature