[Qemu-devel] [PATCH v2 0/1] block: clarify error message for qmp-eject

[Qemu-devel] [PATCH v2 0/1] block: clarify error message for qmp-eject

[John Snow]

v2: Reduce helper to just one parameter,
    push has_force logic back up into qmp interfaces.

    Always return -errno if we set errp,
    return +errno on soft errors where errp remains unset.

John Snow (1):
  block: clarify error message for qmp-eject

 blockdev.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)

-- 
2.4.11

[Qemu-devel] [PATCH v2 1/1] block: clarify error message for qmp-eject

[John Snow]

If you use HMP's eject but the CDROM tray is locked, you may get a
confusing error message informing you that the "tray isn't open."

As this is the point of eject, we can do a little better and help
clarify that the tray was locked and that it (might) open up later,
so try again.

It's not ideal, but it makes the semantics of the (legacy) eject
command more understandable to end users when they try to use it.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1892b8e..4bd94b7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2290,16 +2290,29 @@ exit:
     block_job_txn_unref(block_job_txn);
 }
 
+static int do_open_tray(const char *device, bool force, Error **errp);
+
 void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
 {
     Error *local_err = NULL;
+    int rc;
 
-    qmp_blockdev_open_tray(device, has_force, force, &local_err);
+    if (!has_force) {
+        force = false;
+    }
+
+    rc = do_open_tray(device, force, &local_err);
     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);
 }
 
@@ -2327,35 +2340,36 @@ void qmp_block_passwd(bool has_device, const char *device,
     aio_context_release(aio_context);
 }
 
-void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
-                            Error **errp)
+/**
+ * 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.
+ */
+static int do_open_tray(const char *device, bool force, Error **errp)
 {
     BlockBackend *blk;
     bool locked;
 
-    if (!has_force) {
-        force = false;
-    }
-
     blk = blk_by_name(device);
     if (!blk) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", device);
-        return;
+        return -ENODEV;
     }
 
     if (!blk_dev_has_removable_media(blk)) {
         error_setg(errp, "Device '%s' is not removable", device);
-        return;
+        return -ENOTSUP;
     }
 
     if (!blk_dev_has_tray(blk)) {
         /* Ignore this command on tray-less devices */
-        return;
+        return ENOSYS;
     }
 
     if (blk_dev_is_tray_open(blk)) {
-        return;
+        return 0;
     }
 
     locked = blk_dev_is_medium_locked(blk);
@@ -2366,6 +2380,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
     if (!locked || force) {
         blk_dev_change_media_cb(blk, false);
     }
+
+    if (locked && !force) {
+        return EINPROGRESS;
+    }
+
+    return 0;
+}
+
+void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
+                            Error **errp)
+{
+    if (!has_force) {
+        force = false;
+    }
+    do_open_tray(device, force, errp);
 }
 
 void qmp_blockdev_close_tray(const char *device, Error **errp)
-- 
2.4.11

Re: [Qemu-devel] [PATCH v2 1/1] block: clarify error message for qmp-eject

[Eric Blake]

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

On 05/18/2016 03:53 PM, John Snow wrote:
> If you use HMP's eject but the CDROM tray is locked, you may get a
> confusing error message informing you that the "tray isn't open."
> 
> As this is the point of eject, we can do a little better and help
> clarify that the tray was locked and that it (might) open up later,
> so try again.
> 
> It's not ideal, but it makes the semantics of the (legacy) eject
> command more understandable to end users when they try to use it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 11 deletions(-)

Still somewhat complicated, but this time the complication is
documented, and the clean <0, 0, and >0 semantics make parsing the
return value a bit easier to understand.  And I don't see any cleaner
way to do it.

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 1/1] block: clarify error message for qmp-eject

[Fam Zheng]

On Wed, 05/18 17:53, John Snow wrote:
> If you use HMP's eject but the CDROM tray is locked, you may get a
> confusing error message informing you that the "tray isn't open."
> 
> As this is the point of eject, we can do a little better and help
> clarify that the tray was locked and that it (might) open up later,
> so try again.
> 
> It's not ideal, but it makes the semantics of the (legacy) eject
> command more understandable to end users when they try to use it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Thanks for addressing my comments! And I like the new message a lot.

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/1] block: clarify error message for qmp-eject

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> If you use HMP's eject but the CDROM tray is locked, you may get a
> confusing error message informing you that the "tray isn't open."
>
> As this is the point of eject, we can do a little better and help
> clarify that the tray was locked and that it (might) open up later,
> so try again.
>
> It's not ideal, but it makes the semantics of the (legacy) eject
> command more understandable to end users when they try to use it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
[...]
> @@ -2327,35 +2340,36 @@ void qmp_block_passwd(bool has_device, const char *device,
>      aio_context_release(aio_context);
>  }
>  
> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
> -                            Error **errp)
> +/**
> + * 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.
> + */

Returning or testing for positive errno instead of a negative one is a
fairly common error.  The more we can restrict use of positive errno
codes to errno itself, the less likely such errors are.

Moreover, I feel fatal vs. non-fatal is not for this function to decide.
It's the caller's business.  I'd return -errno on any error.  If you
need this function to also set an error, because it can do a better job
than its callers, then set it on any error.  If a caller wants to
suppress a certain error, it can simply free the error.  Clean
separation of concerns, and a simpler interface.

> +static int do_open_tray(const char *device, bool force, Error **errp)
>  {
>      BlockBackend *blk;
>      bool locked;
>  
> -    if (!has_force) {
> -        force = false;
> -    }
> -
>      blk = blk_by_name(device);
>      if (!blk) {
>          error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>                    "Device '%s' not found", device);
> -        return;
> +        return -ENODEV;
>      }
>  
>      if (!blk_dev_has_removable_media(blk)) {
>          error_setg(errp, "Device '%s' is not removable", device);
> -        return;
> +        return -ENOTSUP;
>      }
>  
>      if (!blk_dev_has_tray(blk)) {
>          /* Ignore this command on tray-less devices */
> -        return;
> +        return ENOSYS;
>      }
>  
>      if (blk_dev_is_tray_open(blk)) {
> -        return;
> +        return 0;
>      }
>  
>      locked = blk_dev_is_medium_locked(blk);
> @@ -2366,6 +2380,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
>      if (!locked || force) {
>          blk_dev_change_media_cb(blk, false);
>      }
> +
> +    if (locked && !force) {
> +        return EINPROGRESS;
> +    }
> +
> +    return 0;
> +}
> +
> +void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
> +                            Error **errp)
> +{
> +    if (!has_force) {
> +        force = false;
> +    }
> +    do_open_tray(device, force, errp);
>  }
>  
>  void qmp_blockdev_close_tray(const char *device, Error **errp)

Re: [Qemu-devel] [PATCH v2 1/1] block: clarify error message for qmp-eject

[John Snow]

On 05/20/2016 10:48 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> If you use HMP's eject but the CDROM tray is locked, you may get a
>> confusing error message informing you that the "tray isn't open."
>>
>> As this is the point of eject, we can do a little better and help
>> clarify that the tray was locked and that it (might) open up later,
>> so try again.
>>
>> It's not ideal, but it makes the semantics of the (legacy) eject
>> command more understandable to end users when they try to use it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> [...]
>> @@ -2327,35 +2340,36 @@ void qmp_block_passwd(bool has_device, const char *device,
>>      aio_context_release(aio_context);
>>  }
>>  
>> -void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
>> -                            Error **errp)
>> +/**
>> + * 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.
>> + */
> 
> Returning or testing for positive errno instead of a negative one is a
> fairly common error.  The more we can restrict use of positive errno
> codes to errno itself, the less likely such errors are.
> 
> Moreover, I feel fatal vs. non-fatal is not for this function to decide.
> It's the caller's business.  I'd return -errno on any error.  If you
> need this function to also set an error, because it can do a better job
> than its callers, then set it on any error.  If a caller wants to
> suppress a certain error, it can simply free the error.  Clean
> separation of concerns, and a simpler interface.
> 
>> +static int do_open_tray(const char *device, bool force, Error **errp)
>>  {
>>      BlockBackend *blk;
>>      bool locked;
>>  
>> -    if (!has_force) {
>> -        force = false;
>> -    }
>> -
>>      blk = blk_by_name(device);
>>      if (!blk) {
>>          error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>>                    "Device '%s' not found", device);
>> -        return;
>> +        return -ENODEV;
>>      }
>>  
>>      if (!blk_dev_has_removable_media(blk)) {
>>          error_setg(errp, "Device '%s' is not removable", device);
>> -        return;
>> +        return -ENOTSUP;
>>      }
>>  
>>      if (!blk_dev_has_tray(blk)) {
>>          /* Ignore this command on tray-less devices */
>> -        return;
>> +        return ENOSYS;
>>      }
>>  
>>      if (blk_dev_is_tray_open(blk)) {
>> -        return;
>> +        return 0;
>>      }
>>  
>>      locked = blk_dev_is_medium_locked(blk);
>> @@ -2366,6 +2380,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
>>      if (!locked || force) {
>>          blk_dev_change_media_cb(blk, false);
>>      }
>> +
>> +    if (locked && !force) {
>> +        return EINPROGRESS;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
>> +                            Error **errp)
>> +{
>> +    if (!has_force) {
>> +        force = false;
>> +    }
>> +    do_open_tray(device, force, errp);
>>  }
>>  
>>  void qmp_blockdev_close_tray(const char *device, Error **errp)

It already got applied, but I can change it to your preference. (Always
return an -errno and an Error, delete-and-free when we don't care about
it...)

--js

Re: [Qemu-devel] [PATCH v2 1/1] block: clarify error message for qmp-eject

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> It already got applied, but I can change it to your preference. (Always
> return an -errno and an Error, delete-and-free when we don't care about
> it...)

I think that would be an improvement.  This is advice, not a demand :)

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/1] block: clarify error message for qmp-eject

[Kevin Wolf]

Am 18.05.2016 um 23:53 hat John Snow geschrieben:
> v2: Reduce helper to just one parameter,
>     push has_force logic back up into qmp interfaces.
> 
>     Always return -errno if we set errp,
>     return +errno on soft errors where errp remains unset.

Thanks, applied to the block branch.

Kevin