Daniel P. Berrangé <berra...@redhat.com> writes: > On Wed, Jun 09, 2021 at 07:34:32PM +0200, Philippe Mathieu-Daudé wrote: >> On 6/9/21 7:27 PM, Philippe Mathieu-Daudé wrote: >> > On 6/9/21 6:01 PM, Marc-André Lureau wrote: >> >> Hi >> >> >> >> On Wed, Jun 9, 2021 at 7:33 PM Philippe Mathieu-Daudé <phi...@redhat.com >> >> <mailto:phi...@redhat.com>> wrote: >> >> >> >> When the management layer queries a binary built using --disable-tpm >> >> for TPM devices, it gets confused by getting empty responses: >> >> >> >> { "execute": "query-tpm" } >> >> { >> >> "return": [ >> >> ] >> >> } >> >> { "execute": "query-tpm-types" } >> >> { >> >> "return": [ >> >> ] >> >> } >> >> { "execute": "query-tpm-models" } >> >> { >> >> "return": [ >> >> ] >> >> } >> >> >> >> Make it clearer by returning an error, mentioning the feature is >> >> disabled: >> >> >> >> { "execute": "query-tpm" } >> >> { >> >> "error": { >> >> "class": "GenericError", >> >> "desc": "this feature or command is not currently supported" >> >> } >> >> } >> >> >> >> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com >> >> <mailto:phi...@redhat.com>> >> >> >> >> >> >> Why not make the qapi schema conditional? >> >> Using your suggestion (and ignoring QAPI marshaling error) I'm getting: >> >> { "execute": "query-tpm" } >> { >> "error": { >> "class": "CommandNotFound", >> "desc": "The command query-tpm has not been found" >> } >> } >> >> Is that OK from a management perspective? > > That's fairly typical of what we'd expect to see from a feature > which is either removed at compile time, or never existed in the first > place. mgmt apps don't really need to distinguish those two scenarios, > so this is fine.
Yes, this is how commands tied to compiled-out features should behave: same both for "this version of QEMU doesn't have the feature" and "this build of QEMU doesn't have the feature". Management applications don't care fore the difference. Unfortunately, quite a few such commands predate QAPI conditionals, and do something else. Making such a command properly conditional now adds a new error to it. We reserve the right to add errors to QMP commands, even though this could conceivably unmask error handling bugs in management applications. I think making such commands properly conditional is a good move anyway, because it makes introspection more useful.
