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
