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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

On 05/17/2016 08:18 PM, Fam Zheng wrote:
> On Tue, 05/17 20:42, John Snow wrote:
>> 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.
>>

>>      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;
> 
> I'm not sure how acceptable it is to leave errp untouched while setting ret
> code to non-zero. Markus?

You're basically returning a tri-state: errp set (fatal, error issued),
0 (success, no error needed), or errp clear and negative (potential
error, but caller must decide).  It's unusual enough that you probably
ought to document it at the top of the function, and it may mess with
Markus' pending work to return status codes from functions taking Error **.

It might also be worth considering using a POSITIVE return value when
not setting an error code, so that a quick <0 check is synonymous with
error set, 0 remains the code for complete success, and the locked tray
with a retry now has an identifiable sentinel - particularly since you
don't care about what errno values were in use except for EINPROGRESS.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]