Re: [Qemu-devel] [PATCH 26/31] tpm: Clean up error reporting in tpm_init_tpmdev()

[Marc-André Lureau]

Hi

On Mon, Oct 8, 2018 at 9:33 PM Markus Armbruster <address@hidden> wrote:
>
> Calling error_report() in a function that takes an Error ** argument
> is suspicious.  tpm_init_tpmdev() does that, and then fails without
> setting an error.  Its caller main(), via tpm_init() and
> qemu_opts_foreach(), is fine with it, but clean it up anyway.
>
> Cc: Stefan Berger <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>

Reviewed-by: Marc-André Lureau <address@hidden>

> ---
>  include/sysemu/tpm.h |  2 +-
>  stubs/tpm.c          |  3 +--
>  tpm.c                | 22 +++++++++-------------
>  vl.c                 |  4 +---
>  4 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index 9ae1ab6da3..17a97ed77a 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -16,7 +16,7 @@
>  #include "qom/object.h"
>
>  int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
> -int tpm_init(void);
> +void tpm_init(void);
>  void tpm_cleanup(void);
>
>  typedef enum TPMVersion {
> diff --git a/stubs/tpm.c b/stubs/tpm.c
> index 6729bc8517..80939cd3db 100644
> --- a/stubs/tpm.c
> +++ b/stubs/tpm.c
> @@ -9,9 +9,8 @@
>  #include "qapi/qapi-commands-tpm.h"
>  #include "sysemu/tpm.h"
>
> -int tpm_init(void)
> +void tpm_init(void)
>  {
> -    return 0;
>  }
>
>  void tpm_cleanup(void)
> diff --git a/tpm.c b/tpm.c
> index 93031723ad..9c9e20bbb7 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -89,19 +89,19 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>      int i;
>
>      if (!QLIST_EMPTY(&tpm_backends)) {
> -        error_report("Only one TPM is allowed.");
> +        error_setg(errp, "Only one TPM is allowed.");
>          return 1;
>      }
>
>      id = qemu_opts_id(opts);
>      if (id == NULL) {
> -        error_report(QERR_MISSING_PARAMETER, "id");
> +        error_setg(errp, QERR_MISSING_PARAMETER, "id");
>          return 1;
>      }
>
>      value = qemu_opt_get(opts, "type");
>      if (!value) {
> -        error_report(QERR_MISSING_PARAMETER, "type");
> +        error_setg(errp, QERR_MISSING_PARAMETER, "type");
>          tpm_display_backend_drivers();
>          return 1;
>      }
> @@ -109,8 +109,8 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>      i = qapi_enum_parse(&TpmType_lookup, value, -1, NULL);
>      be = i >= 0 ? tpm_be_find_by_type(i) : NULL;
>      if (be == NULL) {
> -        error_report(QERR_INVALID_PARAMETER_VALUE,
> -                     "type", "a TPM backend type");
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
> +                   "a TPM backend type");
>          tpm_display_backend_drivers();
>          return 1;
>      }
> @@ -118,7 +118,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, 
> Error **errp)
>      /* validate backend specific opts */
>      qemu_opts_validate(opts, be->opts, &local_err);
>      if (local_err) {
> -        error_report_err(local_err);
> +        error_propagate(errp, local_err);
>          return 1;
>      }
>
> @@ -151,14 +151,10 @@ void tpm_cleanup(void)
>   * Initialize the TPM. Process the tpmdev command line options describing the
>   * TPM backend.
>   */
> -int tpm_init(void)
> +void tpm_init(void)
>  {
> -    if (qemu_opts_foreach(qemu_find_opts("tpmdev"),
> -                          tpm_init_tpmdev, NULL, NULL)) {
> -        return -1;
> -    }
> -
> -    return 0;
> +    qemu_opts_foreach(qemu_find_opts("tpmdev"),
> +                      tpm_init_tpmdev, NULL, &error_fatal);
>  }
>
>  /*
> diff --git a/vl.c b/vl.c
> index 5c159ca45c..b6f9212f09 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4312,9 +4312,7 @@ int main(int argc, char **argv, char **envp)
>                        user_creatable_add_opts_foreach,
>                        object_create_delayed, &error_fatal);
>
> -    if (tpm_init() < 0) {
> -        exit(1);
> -    }
> +    tpm_init();
>
>      /* init the bluetooth world */
>      if (foreach_device_config(DEV_BT, bt_parse))
> --
> 2.17.1
>
>


-- 
Marc-André Lureau