Sorry for the late answer.
Vladimir Sementsov-Ogievskiy <vsementsov@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