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

[John Snow]

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.

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?