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

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

[Philippe Mathieu-Daudé]

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>
---
 stubs/tpm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/stubs/tpm.c b/stubs/tpm.c
index 9bded191d9d..8c904215b39 100644
--- a/stubs/tpm.c
+++ b/stubs/tpm.c
@@ -7,6 +7,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/qapi-commands-tpm.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/error.h"
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
 
@@ -21,16 +23,19 @@ void tpm_cleanup(void)
 
 TPMInfoList *qmp_query_tpm(Error **errp)
 {
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
 
 TpmTypeList *qmp_query_tpm_types(Error **errp)
 {
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
 
 TpmModelList *qmp_query_tpm_models(Error **errp)
 {
+    error_setg(errp, QERR_UNSUPPORTED);
     return NULL;
 }
 
-- 
2.31.1

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi

On Wed, Jun 9, 2021 at 7:33 PM Philippe Mathieu-Daudé <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>
>

Why not make the qapi schema conditional?

---
>  stubs/tpm.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/stubs/tpm.c b/stubs/tpm.c
> index 9bded191d9d..8c904215b39 100644
> --- a/stubs/tpm.c
> +++ b/stubs/tpm.c
> @@ -7,6 +7,8 @@
>
>  #include "qemu/osdep.h"
>  #include "qapi/qapi-commands-tpm.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qapi/error.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
>
> @@ -21,16 +23,19 @@ void tpm_cleanup(void)
>
>  TPMInfoList *qmp_query_tpm(Error **errp)
>  {
> +    error_setg(errp, QERR_UNSUPPORTED);
>      return NULL;
>  }
>
>  TpmTypeList *qmp_query_tpm_types(Error **errp)
>  {
> +    error_setg(errp, QERR_UNSUPPORTED);
>      return NULL;
>  }
>
>  TpmModelList *qmp_query_tpm_models(Error **errp)
>  {
> +    error_setg(errp, QERR_UNSUPPORTED);
>      return NULL;
>  }
>
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2811 bytes --]

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

[Philippe Mathieu-Daudé]

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?

I'm getting:

qapi/qapi-commands-tpm.c:123:13: error: ‘qmp_marshal_output_TPMInfoList’
defined but not used [-Werror=unused-function]
  123 | static void qmp_marshal_output_TPMInfoList(TPMInfoList *ret_in,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
qapi/qapi-commands-tpm.c:73:13: error: ‘qmp_marshal_output_TpmTypeList’
defined but not used [-Werror=unused-function]
   73 | static void qmp_marshal_output_TpmTypeList(TpmTypeList *ret_in,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
qapi/qapi-commands-tpm.c:23:13: error: ‘qmp_marshal_output_TpmModelList’
defined but not used [-Werror=unused-function]
   23 | static void qmp_marshal_output_TpmModelList(TpmModelList *ret_in,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Fixed doing:

-- >8 --
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 0e13d510547..85e332a5979 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -91,6 +91,7 @@ def gen_call(name: str,
 def gen_marshal_output(ret_type: QAPISchemaType) -> str:
     return mcgen('''

+__attribute__((unused))
 static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
                                 QObject **ret_out, Error **errp)
 {
---

But I doubt this is correct... I suppose gen_marshal_output() should
be elided if no command use the type? The enum is used however:

include/sysemu/tpm.h-37-struct TPMIfClass {
include/sysemu/tpm.h-38-    InterfaceClass parent_class;
include/sysemu/tpm.h-39-
include/sysemu/tpm.h:40:    enum TpmModel model;
include/sysemu/tpm.h-41-    void (*request_completed)(TPMIf *obj, int ret);
include/sysemu/tpm.h-42-    enum TPMVersion (*get_version)(TPMIf *obj);
include/sysemu/tpm.h-43-};
include/sysemu/tpm.h-44-

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

[Philippe Mathieu-Daudé]

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?

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

[Daniel P. Berrangé]

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.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Philippe Mathieu-Daudé]

On 6/9/21 7:36 PM, Daniel P. Berrangé wrote:
> 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.

Thank you!

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.