[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_sn
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_snapshot_delete() and related functions |
Date: |
Tue, 16 Apr 2013 11:14:12 -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 | 22 ++++++++++++++--------
> block/qcow2-snapshot.c | 21 +++++++++++++++------
> block/qcow2.h | 4 +++-
> block/rbd.c | 11 ++++++++---
> block/sheepdog.c | 6 ++++--
> include/block/block.h | 4 +++-
> include/block/block_int.h | 4 +++-
> qemu-img.c | 9 ++++-----
> savevm.c | 26 ++++++++++----------------
> 9 files changed, 64 insertions(+), 43 deletions(-)
>
> @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const
> char *snapshot_id)
> /* Search the snapshot */
> snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> if (snapshot_index < 0) {
> - return -ENOENT;
> + error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name",
> + snapshot_id);
> + return;
I haven't looked if you changed this later in the series to have
find_snapshot_by_id_or_name take an errp parameter, at which point you
could refactor the error reporting to that point in the stack. That
might be a nice followup, but this patch is okay as-is.
> +++ b/block/sheepdog.c
> @@ -1908,10 +1908,12 @@ out:
> return ret;
> }
>
> -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + Error **errp)
> {
> /* FIXME: Delete specified snapshot id. */
> - return 0;
> + return;
No semantic change; but a trailing return; in a void function is not
necessary, and an empty body would suffice. On the other hand, claiming
success when we didn't delete anything feels wrong. Should we change
this function to call error_setg() and warn the user that deletion is
not supported at this time? If so, that's probably better done as a
separate commit.
As my findings are best addressed in other patches, I'm okay with this
patch as-is, and you can use:
Reviewed-by: Eric Blake <address@hidden>
--
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 01/11] qemu-img: introduce qemu_img_handle_error(), (continued)
[Qemu-devel] [PATCH 03/11] savevm: update bdrv_snapshot_find() to find snapshot by id or name, Pavel Hrdina, 2013/04/16
[Qemu-devel] [PATCH 04/11] qapi: Convert delvm, Pavel Hrdina, 2013/04/16
[Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_snapshot_delete() and related functions, Pavel Hrdina, 2013/04/16
[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