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

[Fam Zheng]

On Wed, 05/18 07:36, 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

I was only talking about the helper function, but that is more of a personal
taste thing.

> 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