Sorry for the late answer.

Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> writes:

> On 07.03.24 12:46, Markus Armbruster wrote:

[...]

>> I appreciate the attempt to curb the spread of DeviceNotFound errors.
>> Two issues:
>>
>> * Copy-pasting find_device_state() with a false argument is an easy
>>   error to make.
>>
>> * Most uses of find_device_state() are via blk_by_qdev_id() and
>>   qmp_get_blk().  Any new uses of qemu_get_blk() will still produce
>>   DeviceNotFound.
>>
>> Hmm.
>
> Hmm. Right. Wait a bit, I can make the change stricter.
>
> Could you still explain (or give a link), why and when we decided to use only 
> GenericError? I think, having different "error-codes" is a good thing, why we 
> are trying to get rid of it?

We actually got rid of most of them years ago :)

But you deserve a more complete answer.

QMP initially specified the following error response[1]:

    2.4.2 error
    -----------
    
    The error response is issued when the command execution could not be
    completed because of an error condition.
    
    The format is:
    
    { "error": { "class": json-string, "data": json-value }, "id": json-value }
    
     Where,
    
    - The "class" member contains the error class name (eg. 
"ServiceUnavailable")
    - The "data" member contains specific error data and is defined in a
      per-command basis, it will be an empty json-object if the error has no 
data
    - The "id" member contains the transaction identification associated with
      the command execution (if issued by the Client)

Note the structure of @data depends on @class.  We documented a
command's possible error classes (well, we tried), but never bothered to
document the @data it comes with.

Documentation deficits aside, this is looks quite expressive.  There are
issues, though:

1. Formatting errors in human-readable form is bothersome, and creates a
   tight coupling between QMP server and client.

   Consider:

    {"class": "DeviceNotFound", "data": {"device": "ide1-cd0"}}

   To format this in human-readable form, you need to know the error.

   The server does.  Fine print: it has a table mapping JSON templates
   to human-readable error message templates.

   The client needs to duplicate this somehow.  If it receives an error
   it doesn't know, all it can do is barf (possibly dressed up) JSON at
   the human.  To avoid that, clients need to track the server closely:
   tight coupling.

2. Errors have notational overhead, which leads to bad errors.

   To create a new error, you have to edit two source files (not
   counting clients).  Strong incentive to reuse existing errors.  Even
   when they don't quite fit.  When a name provided by the user couldn't
   be resolved, reusing DeviceNotFound is easier than creating a new
   error that is more precise.

3. The human-readable error message is hidden from the programmer's
   view, which leads to bad error messages.

   At the point in the source where the error is created, we see
   something like QERR_DEVICE_NOT_FOUND, name.  To see the
   human-readable message, we have to look up macro
   QERR_DEVICE_NOT_FOUND's error message template in the table, or
   actually test (*gasp*) the error.  People generally do neither, and
   bad error messages proliferate.

4. Too little gain for the pain

   Clients rarely want to handle different errors differently.  More
   often than not, all they do with @class and @data is log them.  Only
   occasionally do they switch on @class, and basically never on @data.

It me took a considerable fight to get the protocol amended to include a
human-readable message[2].  This addressed issue 1.

Over the next two years or so, issues 2. to 4. slowly sank in.  We
eventually tired of the complexity, ripped out @data, and dumbed down
all error classes to GenericError, except for the few clients actually
cared for[3].  We also mandated that new errors avoid the QERR_ macros.

Eliminating the existing QERR_ macros has been slow.  We're down to 13
in master, with patches deleting 7 on the list.

This has served us reasonably well.

Questions?


[1] Commit f544d174dfc
    QMP: Introduce specification
    Dec 2009

[2] Commit 77e595e7c61q
    QMP: add human-readable description to error response
    Dec 2009

[3] Commit de253f14912
    qmp: switch to the new error format on the wire
    Aug 2012


Reply via email to