Re: [Qemu-devel] [PATCH v2] blockdev: clean up error handling in do_open_tray

[Eric Blake <eblake@redhat.com>]

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

On 06/03/2016 01:39 PM, Colin Lord wrote:
> Returns negative error codes and accompanying error messages in cases where
> the device has no tray or the tray is locked and isn't forced open. This
> extra information should result in better flexibility in functions that
> call do_open_tray.
> 
> Signed-off-by: Colin Lord <clord@redhat.com>
> ---
> v2: fix function documentation, improve commit wording, and remove
> unnecessary null check
>  blockdev.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 

> +++ b/blockdev.c
> @@ -2286,17 +2286,14 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>      }
>  
>      rc = do_open_tray(device, force, &local_err);
> -    if (local_err) {
> +    if (rc == -ENOSYS) {
> +        error_free(local_err);
> +        local_err = NULL;
> +    } else if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    if (rc == EINPROGRESS) {
> -        error_setg(errp, "Device '%s' is locked and force was not specified, "
> -                   "wait for tray to open and try again", device);
> -        return;
> -    }
> -
>      qmp_x_blockdev_remove_medium(device, errp);
>  }

The local_err = NULL line is dead code, since nothing else references
local_err before it goes out of scope.  Maintainer could fix that on commit.

>  
> @@ -2325,10 +2322,9 @@ void qmp_block_passwd(bool has_device, const char *device,
>  }
>  
>  /**
> - * returns -errno on fatal error, +errno for non-fatal situations.
> - * errp will always be set when the return code is negative.
> - * May return +ENOSYS if the device has no tray,
> - * or +EINPROGRESS if the tray is locked and the guest has been notified.
> + * returns -errno on all errors, and errp will be set on error
> + * May return the non-fatal error codes -ENOSYS if the device has no tray,
> + * or -EINPROGRESS if the tray is locked and the guest has been notified.

Maybe:

Callers may choose whether to treat -ENOSYS (device has no tray) or
-EINPROGRESS (tray is locked but guest has been notified) as non-fatal
errors.

But what you have works, enough that I'm okay with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 604 bytes --]