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

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 ?

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();
    }

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.