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

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> 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.

My personal taste is to omit unnecessary has_FOOs.

>> 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.)

C is a spartan language.  We can struggle against this and try to build
imitations of what richer languages provide.  That way is Greenspun's
tenth.  I feel it's best to embrace C's spartan nature.

Integer error codes are as spartan as it gets.

You can use a common fixed set like POSIX errno codes.  The fixed codes
will rarely fit exactly, but you can shoehorn them into service often
enough.  Document how you use them, hold your nose if you must.

You can also use function-specific sets, like getaddrinfo()'s EAI_
codes.  Try not to invent too many of them.

You can try to dream up a way to define an integer error code extension
mechanism, but that's not spartan, sorry.

A single Error type encapsulating a human-readable message is
differently spartan.  The spot detecting an error commonly lacks context
to know how to handle it, and the spot handling it commonly lacks detail
to create a decent error message.  This Error type lets the former
create an error message, so the latter doesn't have to.

Occasionally, you need to handle different errors differently, and then
this basic Error type is of no help.

Sometimes, you need to because you created a do_everything() function
that can consequently fail in every imaginable way.  Splitting it up can
make the error handling problem go away.  But sometimes, it refuses to
go away.  Then you need to add an error code somehow.

Python and many other languages use subtyping to let you add arbitrary
data to an error.  Plus, since you can match the type itself, you don't
need to add an error code, just use the type.  Duplicating that in C is
not spartan, sorry.

Spartans get to add the error code to the error object or return it
separately.

GLib adds two integer codes: domain and code.  This is basically an
extensible set of fixed sets.  Pushing the limits of spartan, if you ask
me.

Our Error has an enumeration code ErrorClass.  A fundamental design
mistake was made early on: each ErrorClass value was tied to a single
error message.  As if a human readable error message was a grudging
concession to human users.  The resulting error messages certainly felt
like it.

We've since servered the connection between ErrorClass value and error
message, deprecated ErrorClass, and got rid of most ErrorClass values.
In the few places where we need to handle different errors differently,
we additionally return an ad hoc error code, commonly -errno.

> 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?

We could revive ErrorClass, or add something new.  Adding errno would be
simple enough.  Not sure it's worth it.