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

[Eric Blake]

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