On 05/18/2016 02:36 AM, Fam Zheng wrote:
> On Wed, 05/18 07:36, Markus Armbruster wrote:
>> Fam Zheng <f...@redhat.com> writes:
>>
>>> 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.
>>>>
>>>> Signed-off-by: John Snow <js...@redhat.com>
>>>> ---
>>>> blockdev.c | 36 +++++++++++++++++++++++++++++-------
>>>> 1 file changed, 29 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 1892b8e..feb8484 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -2290,16 +2290,26 @@ exit:
>>>> block_job_txn_unref(block_job_txn);
>>>> }
>>>>
>>>> +static int do_open_tray(const char *device, bool has_force, bool force,
>>>> + Error **errp);
>>>> +
>>>> void qmp_eject(const char *device, bool has_force, bool force, Error
>>>> **errp)
>>>> {
>>>> Error *local_err = NULL;
>>>> + int rc;
>>>>
>>>> - qmp_blockdev_open_tray(device, has_force, force, &local_err);
>>>> + rc = do_open_tray(device, has_force, force, &local_err);
>>>> if (local_err) {
>>>> error_propagate(errp, local_err);
>>>> return;
>>>> }
>>>>
>>>> + if (rc == -EINPROGRESS) {
>>>> + error_setg(errp, "Device '%s' is locked and force was not
>>>> specified, "
>>>> + "wait for tray to open and try again", device);
>>>> + return;
>>>> + }
>>>> +
>>>> qmp_x_blockdev_remove_medium(device, errp);
>>>> }
>>>>
>>>> @@ -2327,8 +2337,8 @@ 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)
>>>> +static int do_open_tray(const char *device, bool has_force, bool force,
>>>> + Error **errp)
>>>
>>> Personally I feel the has_force and force could be merged as one parameter.
>>
>> For qmp_blockdev_open_tray(), the signature is dictated by
>> scripts/qapi-commands.py. To make has_FOO go away, you need to make the
>> FOO non-optional.
>>
>> You have to duplicate the cumbersome has_FOO, FOO couple in your helper
>> functions only when an absent value (has_FOO=false) has special meaning
>
> I was only talking about the helper function, but that is more of a personal
> taste thing.
>
The single solitary reason I didn't squash them for the helper was so
that the git diff looked smaller (The new do_eject is functionally
identical to the old qmp_blockdev_open_tray.)
>> you can't get with any present value. Not my favorite interface design,
>> by the way.
>>
>> We've discussed two improvements to the QAPI language and generators:
>>
>> * Optional with default: has_FOO goes away, and instead FOO assumes the
>> default value declared in the schema when it's absent. Optional
>> without default stays at it is, i.e. has_FOO tells whether it's
>> present.
>>
>> * Use null pointer for absent when it can't be a value.
>>
>> If Eric stops flooding me with QAPI patches, I might even get to
>> implement them :)
>>
>>>> {
>>>> BlockBackend *blk;
>>>> bool locked;
>>>> @@ -2341,21 +2351,21 @@ void qmp_blockdev_open_tray(const char *device,
>>>> bool has_force, bool force,
>>>> 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;
>>>
>>> I'm not sure how acceptable it is to leave errp untouched while setting ret
>>> code to non-zero. Markus?
>>
>> It's questionable style, becaue it gives the two plausible ways to check
>> for errors different meaning:
>>
>> if (do_open_tray(...) < 0) ...
>>
>> and
>>
>> Error *err = NULL;
>> do_open_tray(..., &err);
>> if (err) ...
>>
>> I find this confusing.
>>
>> The former way lets me pass a null Error * argument, which is convenient
>> when I'm not interested in error details.
>>
>> Whenever practical, separate an Error-setting function's values into
>> distinct error and success sets. Example: when a function looks up
>> something, return pointer to it on success, set error and return null on
>> failure.
>>
>> This isn't always practical, for instance, when a pointer-valued
>> function can legitimately return null. That causes confusion, too. We
>> fixed a few bugs around such functions.
>>
>> Whether it isn't practical for *this* function I can't say without
>> developing a better understanding of its purpose and context.
>
> We have this question because errp is mostly human oriented, whereas return
> codes are also used for control logic. From an error pointer a caller can only
> tell if the called function succeeded or not, but cannot tell which type the
> failure is. Comparing this to exception handling systems in other OO
> languages
> such as Python, I feel this is because lacking of the type information which
> would cover this case if we had one too. With error type information, the
> idiom with "ret code + errp" would then become similar to:
>
> try:
> do_open_tray()
> except EjectInProgress:
> pass
> except Exception:
> # report error
> ...
>
> And a return code is not needed. (not saying this is the only type of control
> flow, Functions looking up something will still return pointers, but on the
> other hand it's possible those function may want to return error type too.)
>
> We used to have rich type errors, which has been abandoned, but I think it
> probably makes some sense to let Error carry a standard error code like
> EINPROGRESS, ENOTSUP, etc?
>
> Error *err = NULL;
> do_open_tray(..., &err);
> if (error_num(err) == EINPROGRESS) {
> ...
> } else{
> ...
> }
>
> Or should we simply use errno in this case?
>
> Fam
>
I can't comment on the historical path that QEMU has taken, but I agree
(Naively?) with pretty much everything you've just written. Perhaps
global, standardized exceptions is too tall of a task to tackle, but you
are right that errors + error codes (or classes) would be useful
precisely for situations like these.
