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

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

[Colin Lord]

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(-)

diff --git a/blockdev.c b/blockdev.c
index 717785e..d50a2a5 100644
--- a/blockdev.c
+++ 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);
 }
 
@@ -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.
  */
 static int do_open_tray(const char *device, bool force, Error **errp)
 {
@@ -2348,8 +2344,8 @@ static int do_open_tray(const char *device, bool force, Error **errp)
     }
 
     if (!blk_dev_has_tray(blk)) {
-        /* Ignore this command on tray-less devices */
-        return ENOSYS;
+        error_setg(errp, "Device '%s' does not have a tray", device);
+        return -ENOSYS;
     }
 
     if (blk_dev_is_tray_open(blk)) {
@@ -2366,7 +2362,9 @@ static int do_open_tray(const char *device, bool force, Error **errp)
     }
 
     if (locked && !force) {
-        return EINPROGRESS;
+        error_setg(errp, "Device '%s' is locked and force was not specified, "
+                   "wait for tray to open and try again", device);
+        return -EINPROGRESS;
     }
 
     return 0;
@@ -2375,10 +2373,18 @@ static int do_open_tray(const char *device, bool force, Error **errp)
 void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
                             Error **errp)
 {
+    Error *local_err = NULL;
+    int rc;
+
     if (!has_force) {
         force = false;
     }
-    do_open_tray(device, force, errp);
+    rc = do_open_tray(device, force, &local_err);
+    if (rc == -EINPROGRESS || rc == -ENOSYS) {
+        error_free(local_err);
+    } else {
+        error_propagate(errp, local_err);
+    }
 }
 
 void qmp_blockdev_close_tray(const char *device, Error **errp)
-- 
2.5.5

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

[Kevin Wolf]

Am 03.06.2016 um 21:39 hat Colin Lord geschrieben:
> 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>

Thanks, applied to the block branch.

Kevin

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

[Eric Blake]

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

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

[Markus Armbruster]

Colin Lord <clord@redhat.com> writes:

> 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(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 717785e..d50a2a5 100644
> --- a/blockdev.c
> +++ 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;
>      }

I like to put the error case in a conditional, and leave the normal flow
unindented, because it makes the normal flow easier to follow:

       if (rc && rc != -ENOSYS) {
           error_propagate(errp, local_err);
           return;
       }
       error_free(local_err);

error_free(NULL) is safe.

>  
> -    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);
>  }
>  
> @@ -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.
>   */

What does the function do?

What does it return on success?

Suggest imperative mood.

Here's my try:

   /*
    * Attempt to open the tray of @device.
    * If @force, ignore its tray lock.
    * Else, if the tray is locked, don't open it, but ask the guest to
    * open it.
    * On error, store an error through @errp and return -errno.
    * If @device does not exist, return -ENODEV.
    * If it has no removable media, return -ENOTSUP.
    * If it has no tray, return -ENOSYS.
    * If the guest was asked to open the tray, return -EINPROGRESS.
    * Else, return 0.
    */

>  static int do_open_tray(const char *device, bool force, Error **errp)
>  {
> @@ -2348,8 +2344,8 @@ static int do_open_tray(const char *device, bool force, Error **errp)
>      }
>  
>      if (!blk_dev_has_tray(blk)) {
> -        /* Ignore this command on tray-less devices */
> -        return ENOSYS;
> +        error_setg(errp, "Device '%s' does not have a tray", device);
> +        return -ENOSYS;
>      }
>  
>      if (blk_dev_is_tray_open(blk)) {
> @@ -2366,7 +2362,9 @@ static int do_open_tray(const char *device, bool force, Error **errp)
>      }
>  
>      if (locked && !force) {
> -        return EINPROGRESS;
> +        error_setg(errp, "Device '%s' is locked and force was not specified, "
> +                   "wait for tray to open and try again", device);
> +        return -EINPROGRESS;
>      }
>  
>      return 0;
> @@ -2375,10 +2373,18 @@ static int do_open_tray(const char *device, bool force, Error **errp)
>  void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
>                              Error **errp)
>  {
> +    Error *local_err = NULL;
> +    int rc;
> +
>      if (!has_force) {
>          force = false;
>      }
> -    do_open_tray(device, force, errp);
> +    rc = do_open_tray(device, force, &local_err);
> +    if (rc == -EINPROGRESS || rc == -ENOSYS) {
> +        error_free(local_err);
> +    } else {
> +        error_propagate(errp, local_err);
> +    }
>  }

Likewise:

       if (rc && rc != -ENOSYS && rc != EINPROGRESS) {
           error_propagate(errp, local_err);
           return;
       }
       error_free(local_err);

>  
>  void qmp_blockdev_close_tray(const char *device, Error **errp)

While you're cleaning up: mind moving the forward declaration of
do_open_tray() to the beginning?