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

[John Snow <jsnow@redhat.com>]

On 05/18/2016 02:36 AM, Fam Zheng wrote:
> On Wed, 05/18 07:36, Markus Armbruster wrote:
>> Fam Zheng <famz@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 <jsnow@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.