[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 05/11] block: update error reporting for bdrv_sn
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 05/11] block: update error reporting for bdrv_snapshot_goto() and related functions |
Date: |
Tue, 16 Apr 2013 14:48:26 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130402 Thunderbird/17.0.5 |
On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
> 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
- Re: [Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_snapshot_delete() and related functions, (continued)
[Qemu-devel] [PATCH 07/11] qapi: Convert loadvm, Pavel Hrdina, 2013/04/16
[Qemu-devel] [PATCH 05/11] block: update error reporting for bdrv_snapshot_goto() and related functions, Pavel Hrdina, 2013/04/16
[Qemu-devel] [PATCH 06/11] savevm: update error reporting for qemu_loadvm_state(), Pavel Hrdina, 2013/04/16
[Qemu-devel] [PATCH 08/11] block: update error reporting for bdrv_snapshot_create() and related functions, Pavel Hrdina, 2013/04/16
[Qemu-devel] [PATCH 11/11] savevm: remove backward compatibility from bdrv_snapshot_find(), Pavel Hrdina, 2013/04/16