Eric Blake <ebl...@redhat.com> 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.