Re: [Qemu-devel] [PATCH for-2.5 0/4] Expose ErrorClass through introspection

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> I noticed that introspection was not documenting either
> qmp_capabilities nor the ErrorClass enum.  I think this is worth
> fixing for 2.5 when introspection is brand new, so that if we later
> extend the ErrorClass enum or add future capability negotiation (and
> in particular if such additions get backported in downstream builds),
> a client will be able to use introspection to learn whether the new
> features are supported, regardless of the qemu version.
>
> Note that this also adds qmp_capabilities to 'query-commands'.
>
> Yes, this is borderline, and you may decide that it doesn't deserve
> to be called a bug and should wait for 2.6.

Before I discuss the error class proposal in more detail, a preliminary
remark: error classes are a leftover from the days of "rich" error
objects, and any new use of an error class other than
ERROR_CLASS_GENERIC_ERROR is immediately suspect.  I'm not saying that
we won't add such uses anymore, just that there's a significant bar to
overcome, which we haven't for quite some time now.

I think I could be persuaded that a client might be able to use
knowledge on what error classes a specific command can produce.  Of
course, presence of an error class doesn't tell what actual error
conditions map to it, i.e. the client still needs to make assumptions on
the meaning of error classes.  Humans make those, too, but humans can
read the contract in the comments.

The value of a global list of error classes seems even more dubious,
though.  Existence of an error class by itself guarantees nothing.  How
would a client use the information?  Assume that existence of a class
implies a certain command uses it in a certain way?  That's an even
bigger jump than above.

I guess using the presence or absence of an error class as a witness for
a certain feature or behavior could work.  Seems practical when the
written contract guarantees the connection between the two (de jure
connection), or the commit that introduces the feature or behavior also
adds or removes the error class (de facto connecton).  This applies both
to a global list of error classes and to per-command lists.

Example 1: MigrationExpected

    Before commit 1e99814 "qmp: handle stop/cont in INMIGRATE state",
    cont could fail with error MigrationExpected.  Libvirt dealt with it
    by trying again.

    Commit 1e99814 made cont just work, and dropped the error class.
    The error class was never used for anything else.

    Exposing a global list of error classes like your patch does would
    permit detecting the presence of this commit.  However, detecting it
    is pointless: to deal with its absence, you have to loop on
    MigrationExpected anyway, and that code works just fine with and
    without the commit.

Example 2: Unwanted DeviceNotFound dropped

    During the 2.3 development cycle, a few unwanted uses of
    DeviceNotFound crept in.  Commits 5b347c5, f3cf80e, 6ec46ad backed
    them out before the release.

    For the sake of argument, ignore the fact that these unwanted
    DeviceNotFound never made it to a release, and if they had, we
    would've left them in, because taking them out would've been an ABI
    break.

    A client could use a per-command error class list to detect them,
    but not a global error class list.  But what could it do with the
    information?  If DeviceNotFound is there, the client can handle it
    specially, if not, it can't, and I can't see how knowing it would
    make a difference.

Example 3 (hypothetical): New error class to support a client's need

    Say we discover that a client wants to handle a certain error
    specially, and we decide to make that possible by changing its error
    class from GenericError to something specific to that error.
    Hypothetical, because changing an error's error class is an ABI
    break, and we normally don't do that.

    The client could then refrain from using the command in certain ways
    unless it uses the specific error class for this error.

    Detecting that by finding the error class in the global list of
    error classes works only if the error class is new, and only works
    as long as it doesn't get used for other things that then get
    backported without the original use.

    Detecting it by finding the error class in the command's list of
    error classes would be less brittle.

Example 4: Use per-command error list to catch unwanted error class

    If we declare a command's errors, we can detect undeclared errors at
    run time.  This should help catching unwanted ones early (see
    example 2).

    Having to declare error classes may facilitate proper review of new
    uses of funky error classes.

None of these examples is a particularly convincing use case.  Can you
think of better ones?

Finally, what happens if error class introspection misses 2.5 and makes
a later version?

If we add a global error class list like this patch does, a client has
to assume that anything that doesn't has one has the usual error
classes: GenericError, CommandNotFound, DeviceEncrypted,
DeviceNotActive, DeviceNotFound, KVMMissingCap[*].  Whether "doesn't has
one" is "doesn't has one in query-qmp-schema" or "doesn't even have
query-qmp-schema" doesn't matter.

If we add per-command error class lists, it's the same, only the
assumptions become a bit more involved.

Of course, the fewer discernible versions of introspection we have, the
better.  If we can figure out what we need in time to get it into the
very first version, great!  But it's awfully late for 2.5, and I'm not
at all sure we know what we need.  Perhaps we can find out quickly, but
let's not rush the job.


[*] Ancient versions may also have MigrationExpected (see above), but
backporting introspection that far, but not backporting the fix for the
migration stop/cont race would be insane.