Re: [PATCH] tpm: Return QMP error when TPM is disabled in build

[Markus Armbruster]

Daniel P. Berrangé <berrange@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é <philmd@redhat.com
>> >> <mailto:philmd@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é <philmd@redhat.com
>> >>     <mailto:philmd@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.