All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tpm: Return QMP error when TPM is disabled in build
@ 2021-06-09 15:25 Philippe Mathieu-Daudé
  2021-06-09 16:01 ` Marc-André Lureau
  0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-09 15:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Stefan Berger

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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] tpm: Return QMP error when TPM is disabled in build
  2021-06-09 15:25 [PATCH] tpm: Return QMP error when TPM is disabled in build Philippe Mathieu-Daudé
@ 2021-06-09 16:01 ` Marc-André Lureau
  2021-06-09 17:27   ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 7+ messages in thread
From: Marc-André Lureau @ 2021-06-09 16:01 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Paolo Bonzini, QEMU, Stefan Berger

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tpm: Return QMP error when TPM is disabled in build
  2021-06-09 16:01 ` Marc-André Lureau
@ 2021-06-09 17:27   ` Philippe Mathieu-Daudé
  2021-06-09 17:34     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-09 17:27 UTC (permalink / raw)
  To: Marc-André Lureau, Markus Armbruster
  Cc: Paolo Bonzini, QEMU, Stefan Berger

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-



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] tpm: Return QMP error when TPM is disabled in build
  2021-06-09 17:27   ` Philippe Mathieu-Daudé
@ 2021-06-09 17:34     ` Philippe Mathieu-Daudé
  2021-06-09 17:36       ` Daniel P. Berrangé
  0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-09 17:34 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: Paolo Bonzini, Stefan Berger, QEMU, Markus Armbruster

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?



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tpm: Return QMP error when TPM is disabled in build
  2021-06-09 17:34     ` Philippe Mathieu-Daudé
@ 2021-06-09 17:36       ` Daniel P. Berrangé
  2021-06-09 17:46         ` Philippe Mathieu-Daudé
  2021-06-10  9:15         ` Markus Armbruster
  0 siblings, 2 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2021-06-09 17:36 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Paolo Bonzini, Marc-André Lureau, QEMU, Markus Armbruster,
	Stefan Berger

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 :|



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tpm: Return QMP error when TPM is disabled in build
  2021-06-09 17:36       ` Daniel P. Berrangé
@ 2021-06-09 17:46         ` Philippe Mathieu-Daudé
  2021-06-10  9:15         ` Markus Armbruster
  1 sibling, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-09 17:46 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Paolo Bonzini, Marc-André Lureau, QEMU, Markus Armbruster,
	Stefan Berger

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!



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] tpm: Return QMP error when TPM is disabled in build
  2021-06-09 17:36       ` Daniel P. Berrangé
  2021-06-09 17:46         ` Philippe Mathieu-Daudé
@ 2021-06-10  9:15         ` Markus Armbruster
  1 sibling, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2021-06-10  9:15 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Paolo Bonzini, Stefan Berger, Philippe Mathieu-Daudé,
	Marc-André Lureau, QEMU

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.



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-06-10  9:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 15:25 [PATCH] tpm: Return QMP error when TPM is disabled in build Philippe Mathieu-Daudé
2021-06-09 16:01 ` Marc-André Lureau
2021-06-09 17:27   ` Philippe Mathieu-Daudé
2021-06-09 17:34     ` Philippe Mathieu-Daudé
2021-06-09 17:36       ` Daniel P. Berrangé
2021-06-09 17:46         ` Philippe Mathieu-Daudé
2021-06-10  9:15         ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.