[PATCH-for-5.1 0/2] tpm: Improve error reporting

[PATCH-for-5.1 0/2] tpm: Improve error reporting

[Philippe Mathieu-Daudé]

Improve error reporting by listing TPM backends.

Philippe Mathieu-Daudé (2):
  tpm: Display when no backend is available
  tpm: List the available TPM backends

 tpm.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

-- 
2.21.3

[PATCH-for-5.1 1/2] tpm: Display when no backend is available

[Philippe Mathieu-Daudé]

Display "No TPM backend available in this binary." error when
no backend is available.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tpm.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tpm.c b/tpm.c
index fe03b24858..e36803a64d 100644
--- a/tpm.c
+++ b/tpm.c
@@ -41,6 +41,22 @@ tpm_be_find_by_type(enum TpmType type)
     return TPM_BACKEND_CLASS(oc);
 }
 
+/*
+ * Walk the list of available TPM backend drivers and count them.
+ */
+static int tpm_backend_drivers_count(void)
+{
+    int count = 0, i;
+
+    for (i = 0; i < TPM_TYPE__MAX; i++) {
+        const TPMBackendClass *bc = tpm_be_find_by_type(i);
+        if (bc) {
+            count++;
+        }
+    }
+    return count;
+}
+
 /*
  * Walk the list of available TPM backend drivers and display them on the
  * screen.
@@ -87,6 +103,11 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
     TPMBackend *drv;
     int i;
 
+    if (!tpm_backend_drivers_count()) {
+        error_setg(errp, "No TPM backend available in this binary.");
+        return 1;
+    }
+
     if (!QLIST_EMPTY(&tpm_backends)) {
         error_setg(errp, "Only one TPM is allowed.");
         return 1;
-- 
2.21.3

Re: [PATCH-for-5.1 1/2] tpm: Display when no backend is available

[Stefan Berger]

On 7/22/20 7:23 AM, Philippe Mathieu-Daudé wrote:
> Display "No TPM backend available in this binary." error when
> no backend is available.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tpm.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/tpm.c b/tpm.c
> index fe03b24858..e36803a64d 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -41,6 +41,22 @@ tpm_be_find_by_type(enum TpmType type)
>       return TPM_BACKEND_CLASS(oc);
>   }
>   
> +/*
> + * Walk the list of available TPM backend drivers and count them.
> + */
> +static int tpm_backend_drivers_count(void)
> +{
> +    int count = 0, i;
> +
> +    for (i = 0; i < TPM_TYPE__MAX; i++) {
> +        const TPMBackendClass *bc = tpm_be_find_by_type(i);
> +        if (bc) {
> +            count++;
> +        }
> +    }
> +    return count;
> +}
> +
>   /*
>    * Walk the list of available TPM backend drivers and display them on the
>    * screen.
> @@ -87,6 +103,11 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>       TPMBackend *drv;
>       int i;
>   
> +    if (!tpm_backend_drivers_count()) {
> +        error_setg(errp, "No TPM backend available in this binary.");
> +        return 1;
> +    }
> +
>       if (!QLIST_EMPTY(&tpm_backends)) {
>           error_setg(errp, "Only one TPM is allowed.");
>           return 1;


Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH-for-5.1 1/2] tpm: Display when no backend is available

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Display "No TPM backend available in this binary." error when
> no backend is available.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tpm.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/tpm.c b/tpm.c
> index fe03b24858..e36803a64d 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -41,6 +41,22 @@ tpm_be_find_by_type(enum TpmType type)
>      return TPM_BACKEND_CLASS(oc);
>  }
>  
> +/*
> + * Walk the list of available TPM backend drivers and count them.
> + */
> +static int tpm_backend_drivers_count(void)
> +{
> +    int count = 0, i;
> +
> +    for (i = 0; i < TPM_TYPE__MAX; i++) {
> +        const TPMBackendClass *bc = tpm_be_find_by_type(i);
> +        if (bc) {
> +            count++;
> +        }
> +    }
> +    return count;
> +}
> +
>  /*
>   * Walk the list of available TPM backend drivers and display them on the
>   * screen.
> @@ -87,6 +103,11 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>      TPMBackend *drv;
>      int i;
>  
> +    if (!tpm_backend_drivers_count()) {
> +        error_setg(errp, "No TPM backend available in this binary.");

Scratch the '.', please.  From error_setg()'s contract:

 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.

> +        return 1;
> +    }
> +
>      if (!QLIST_EMPTY(&tpm_backends)) {
>          error_setg(errp, "Only one TPM is allowed.");
>          return 1;

This works, but it's more code than I'd like to see for the purpose.

Moreover, it's tied to the error handling issue discussed in

    Subject: Re: What is TYPE_TPM_TIS_ISA? (Not an ISA Device)
    Message-ID: <87tuxyoauy.fsf@dusky.pond.sub.org>

If we revert flawed commit d10e05f15d5, then your patch needs a v2.
Your PATCH 2 might become unnecessary.  I'll give it a quick try to see
how it comes out.

[RFC PATCH-for-5.1 2/2] tpm: List the available TPM backends

[Philippe Mathieu-Daudé]

When an incorrect backend is selected, tpm_display_backend_drivers()
is supposed to list the available backends. However the error is
directly propagated, and we never display the list. The user only
gets "Parameter 'type' expects a TPM backend type" error.

Convert the fprintf(stderr,) calls to error hints propagated with
the error.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because this is now odd in tpm_config_parse():

  tpm_list_backend_drivers_hint(&error_fatal);
  return -1;
---
 tpm.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tpm.c b/tpm.c
index e36803a64d..358566cb10 100644
--- a/tpm.c
+++ b/tpm.c
@@ -58,23 +58,21 @@ static int tpm_backend_drivers_count(void)
 }
 
 /*
- * Walk the list of available TPM backend drivers and display them on the
- * screen.
+ * Walk the list of available TPM backend drivers and list them as Error hint.
  */
-static void tpm_display_backend_drivers(void)
+static void tpm_list_backend_drivers_hint(Error **errp)
 {
     int i;
 
-    fprintf(stderr, "Supported TPM types (choose only one):\n");
+    error_append_hint(errp, "Supported TPM types (choose only one):\n");
 
     for (i = 0; i < TPM_TYPE__MAX; i++) {
         const TPMBackendClass *bc = tpm_be_find_by_type(i);
         if (!bc) {
             continue;
         }
-        fprintf(stderr, "%12s   %s\n", TpmType_str(i), bc->desc);
+        error_append_hint(errp, "%12s   %s\n", TpmType_str(i), bc->desc);
     }
-    fprintf(stderr, "\n");
 }
 
 /*
@@ -97,6 +95,7 @@ TPMBackend *qemu_find_tpm_be(const char *id)
 
 static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
 {
+    ERRP_GUARD();
     const char *value;
     const char *id;
     const TPMBackendClass *be;
@@ -122,7 +121,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
     value = qemu_opt_get(opts, "type");
     if (!value) {
         error_setg(errp, QERR_MISSING_PARAMETER, "type");
-        tpm_display_backend_drivers();
+        tpm_list_backend_drivers_hint(errp);
         return 1;
     }
 
@@ -131,7 +130,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
     if (be == NULL) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
                    "a TPM backend type");
-        tpm_display_backend_drivers();
+        tpm_list_backend_drivers_hint(errp);
         return 1;
     }
 
@@ -184,7 +183,7 @@ int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
     QemuOpts *opts;
 
     if (!strcmp(optarg, "help")) {
-        tpm_display_backend_drivers();
+        tpm_list_backend_drivers_hint(&error_fatal);
         return -1;
     }
     opts = qemu_opts_parse_noisily(opts_list, optarg, true);
-- 
2.21.3

Re: [RFC PATCH-for-5.1 2/2] tpm: List the available TPM backends

[Stefan Berger]

On 7/22/20 7:23 AM, Philippe Mathieu-Daudé wrote:
> When an incorrect backend is selected, tpm_display_backend_drivers()
> is supposed to list the available backends. However the error is
> directly propagated, and we never display the list. The user only
> gets "Parameter 'type' expects a TPM backend type" error.
>
> Convert the fprintf(stderr,) calls to error hints propagated with
> the error.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because this is now odd in tpm_config_parse():

because it's not using the fprintf anymore ?

Re: [RFC PATCH-for-5.1 2/2] tpm: List the available TPM backends

[Philippe Mathieu-Daudé]

On 7/22/20 11:44 PM, Stefan Berger wrote:
> On 7/22/20 7:23 AM, Philippe Mathieu-Daudé wrote:
>> When an incorrect backend is selected, tpm_display_backend_drivers()
>> is supposed to list the available backends. However the error is
>> directly propagated, and we never display the list. The user only
>> gets "Parameter 'type' expects a TPM backend type" error.
>>
>> Convert the fprintf(stderr,) calls to error hints propagated with
>> the error.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> RFC because this is now odd in tpm_config_parse():
> 
> because it's not using the fprintf anymore ?
> 
> 

Because when using &error_fatal you don't return:

    if (!strcmp(optarg, "help")) {
        tpm_list_backend_drivers_hint(&error_fatal);
        /* not reached */
        return -1;
    }

I should probably use that instead:

    if (!strcmp(optarg, "help")) {
        tpm_list_backend_drivers_hint(&error_fatal);
        g_assert_not_reached();
    }

Re: [RFC PATCH-for-5.1 2/2] tpm: List the available TPM backends

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> When an incorrect backend is selected, tpm_display_backend_drivers()
> is supposed to list the available backends. However the error is
> directly propagated, and we never display the list. The user only
> gets "Parameter 'type' expects a TPM backend type" error.
>
> Convert the fprintf(stderr,) calls to error hints propagated with
> the error.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because this is now odd in tpm_config_parse():
>
>   tpm_list_backend_drivers_hint(&error_fatal);
>   return -1;
> ---
>  tpm.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/tpm.c b/tpm.c
> index e36803a64d..358566cb10 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -58,23 +58,21 @@ static int tpm_backend_drivers_count(void)
>  }
>  
>  /*
> - * Walk the list of available TPM backend drivers and display them on the
> - * screen.
> + * Walk the list of available TPM backend drivers and list them as Error hint.
>   */
> -static void tpm_display_backend_drivers(void)
> +static void tpm_list_backend_drivers_hint(Error **errp)
>  {
>      int i;
>  
> -    fprintf(stderr, "Supported TPM types (choose only one):\n");
> +    error_append_hint(errp, "Supported TPM types (choose only one):\n");
>  
>      for (i = 0; i < TPM_TYPE__MAX; i++) {
>          const TPMBackendClass *bc = tpm_be_find_by_type(i);
>          if (!bc) {
>              continue;
>          }
> -        fprintf(stderr, "%12s   %s\n", TpmType_str(i), bc->desc);
> +        error_append_hint(errp, "%12s   %s\n", TpmType_str(i), bc->desc);
>      }
> -    fprintf(stderr, "\n");
>  }
>  
>  /*
> @@ -97,6 +95,7 @@ TPMBackend *qemu_find_tpm_be(const char *id)
>  
>  static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>  {
> +    ERRP_GUARD();
>      const char *value;
>      const char *id;
>      const TPMBackendClass *be;
> @@ -122,7 +121,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>      value = qemu_opt_get(opts, "type");
>      if (!value) {
>          error_setg(errp, QERR_MISSING_PARAMETER, "type");
> -        tpm_display_backend_drivers();
> +        tpm_list_backend_drivers_hint(errp);
>          return 1;
>      }
>  

Yes, this is how we should list available backends together with
error_setg().  Simply printing them to stderr is wrong then.

> @@ -131,7 +130,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp)
>      if (be == NULL) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
>                     "a TPM backend type");
> -        tpm_display_backend_drivers();
> +        tpm_list_backend_drivers_hint(errp);
>          return 1;
>      }
>  
> @@ -184,7 +183,7 @@ int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
>      QemuOpts *opts;
>  
>      if (!strcmp(optarg, "help")) {
> -        tpm_display_backend_drivers();
> +        tpm_list_backend_drivers_hint(&error_fatal);
>          return -1;
>      }
>      opts = qemu_opts_parse_noisily(opts_list, optarg, true);

A bit worse than weird:

    $ qemu-system-x86_64 -tpmdev help
    upstream-qemu: /work/armbru/qemu/util/error.c:158: error_append_hint: Assertion `err && errp != &error_abort && errp != &error_fatal' failed.
    Aborted (core dumped)

If we choose to use Error here, then I'd recommend two functions:

1. One to append a *short* hint.  Something like this:

    qemu-system-x86_64: -tpmdev xxx,id=tpm0: Parameter 'type' expects a TPM backend type
    Supported TPM types are passthrough, emulator.

   Actually, I wouldn't even make it a function, but simply do it inline
   for the "invalid value" case.  The missing value case can do without.
   Matter of taste.

2. Another one to print help.

Let's first decide whether to revert commit d10e05f15d5 instead.