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

[John Snow <jsnow@redhat.com>]

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

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