[Qemu-devel] [PATCH] blockdev: fix error handling in do_open_tray

[Qemu-devel] [PATCH] blockdev: fix error handling in do_open_tray

[clord]

From: Colin Lord <clord@redhat.com>

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>
---
 blockdev.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 717785e..d10ab35 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);
 }
 
@@ -2348,8 +2345,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 +2363,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 +2374,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 (local_err && (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] blockdev: fix error handling in do_open_tray

[Eric Blake]

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

On 06/03/2016 12:20 PM, clord@redhat.com wrote:
> From: Colin Lord <clord@redhat.com>

You may want to update your ~/.gitconfig to set sendemail.from to list
your name in the same manner in which you add S-o-b.  (Not strictly a
problem, as 'git am' does the right thing either way, but it will avoid
this secondary From: line in patch mails you send)

> 
> 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>
> ---
>  blockdev.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 717785e..d10ab35 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;
> -    }
> -

So in this function, we still ignore ENOSYS, and the EINPROGRESS error
message has now moved to the helper function. No change in overall
behavior, good.

>      qmp_x_blockdev_remove_medium(device, errp);
>  }
>  
> @@ -2348,8 +2345,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 +2363,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;

And here, we always return negative error.

Oops, you forgot to update the comments before this function, describing
its new (simpler) semantics.

>      }
>  
>      return 0;
> @@ -2375,10 +2374,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 (local_err && (rc == -EINPROGRESS || rc == -ENOSYS)) {

The 'local_err &&' portion of the conditional is dead code. You could go
straight into the comparison of particular rc values.

> +        error_free(local_err);
> +    } else {
> +        error_propagate(errp, local_err);
> +    }
>  }
>  
>  void qmp_blockdev_close_tray(const char *device, Error **errp)
> 

This appears to be your first qemu patch - congratulations, and welcome
to the community.

Looking forward to v2.

-- 
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] blockdev: fix error handling in do_open_tray

[Kevin Wolf]

Am 03.06.2016 um 20:20 hat clord@redhat.com geschrieben:
> From: Colin Lord <clord@redhat.com>
> 
> 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>

Strictly speaking not a fix, but just a cleanup, right? Maybe change
that in the subject line.

> @@ -2348,8 +2345,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 +2363,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;

You're changing the return values here (which is fine), but we need to
update the function comment as well. It still says:

 * May return +ENOSYS if the device has no tray,
 * or +EINPROGRESS if the tray is locked and the guest has been notified.

> @@ -2375,10 +2374,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 (local_err && (rc == -EINPROGRESS || rc == -ENOSYS)) {

rc < 0 already implies that local_err is set, so while that check
doesn't hurt, technically it's redundant.

> +        error_free(local_err);
> +    } else {
> +        error_propagate(errp, local_err);
> +    }
>  }

Kevin