Re: [Qemu-devel] [PATCH v2 09/12] block: update return value from bdrv_snapshot_create

[Eric Blake <eblake@redhat.com>]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> If we provide error message, we should also provide a return code.
> In some cases we could not care about any error message and the return
> code is enough for as.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---

> +++ b/qemu-img.c
> @@ -1943,6 +1943,7 @@ static int img_snapshot(int argc, char **argv)
>      int action = 0;
>      qemu_timeval tv;
>      bool quiet = false;
> +    Error *local_err;
>  
>      bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
>      /* Parse commandline parameters */
> @@ -2019,10 +2020,10 @@ static int img_snapshot(int argc, char **argv)
>          sn.date_sec = tv.tv_sec;
>          sn.date_nsec = tv.tv_usec * 1000;
>  
> -        ret = bdrv_snapshot_create(bs, &sn, NULL);
> +        local_err = NULL;
> +        ret = bdrv_snapshot_create(bs, &sn, &local_err);
>          if (ret) {

Isn't it true that 'ret' is non-zero iff 'local_err' is no longer NULL?
 In which case, why not have the functions return 'void' instead of an
integer, and use the null-ness of local_err be your test of whether
bdrv_snapshot_create failed.

> -            error_report("Could not create snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> +            qemu_img_handle_error(local_err);

Same question as in 8/12 - you have already guaranteed that
qemu_img_handle_error will only be called if local_err is nonnull, so
does that helper function have to do a sanity check?

-- 
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 --]