John Snow <js...@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 <js...@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)
