On 05/20/2016 10:48 AM, Markus Armbruster wrote: > 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)
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