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

[Markus Armbruster <armbru@redhat.com>]

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)