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)