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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 05/18/2016 01:36 AM, Markus Armbruster wrote:
>> Fam Zheng <address@hidden> 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 <address@hidden>
>>>> ---
>>>>  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
>> 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.
>> 
>> [...]
>> 
>
> Basically, in some contexts certain callers *may* consider certain
> error/success conditions as an error, and in others they may not.

This is an instance of the general pattern: callee knows exactly what
went wrong, but not how to handle it.  The place that can decide how to
handle it is up-stack, where we don't know much about what went wrong
anymore.

The current code comonly solves it like this.  The callee returns an
error code and sets an error.  Code and error get propagated to the
place that can decide.  Said place examines the code.  It may decide
that this isn't an error after all.  It probably has no use for the
error then, so it frees it.

> For instance, when using qmp_blockdev_open_tray directly via QMP, it has
> a few outcomes:
>
> 1) Tray is unlocked, force parameter is irrelevant, command succeeds.
> 2) Tray is locked, force is false, command "fails," but Guest VM is
> notified of a desire to open the tray and may or may not respect the
> command. We have "successfully" applied a best effort to opening the tray.
> 3) Tray is locked, force is true, command succeeds.
>
> It's the behavior of #2 that I am trying to clarify here.
>
> When used indirectly via qmp_eject, #2 is definitely an error -- the
> full routine of eject will NOT succeed (we cannot remove the medium) so
> the error reported to the user should be from Eject's perspective, not
> medium_remove's.
>
> To this end, I thought I'd add error codes into the helper to help
> callers differentiate hard errors from "soft errors."
>
> If this is too iffy for people, I can:
>
> - Leave all error codes set to -ERRNO if we set errp, and
> - Change all "maybe error codes" +ERRNO if we don't set errp. (Eric's
> suggestion.)
>
> Good?

If both X and -X occur for some value of X, this is confusing.  But I
guess (hope!) no such X exists.

If only one of them can occur, then this is basically using the sign to
encode the callee's best guess on what callers may wish to treat as an
error.  I find that... unusual.  Wouldn't do it.  I'd leave the decision
to the caller, and keep the interface simple.