Re: [Qemu-devel] [PATCH 26/31] tpm: Clean up error reporting in tpm_init_tpmdev()
On 10/8/18 1:31 PM, Markus Armbruster 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 Signed-off-by: Markus Armbruster Reviewed-by: Stefan Berger
Re: [Qemu-devel] [PATCH 26/31] tpm: Clean up error reporting in tpm_init_tpmdev()
Hi
On Mon, Oct 8, 2018 at 9:33 PM Markus Armbruster 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
> Signed-off-by: Markus Armbruster
Reviewed-by: Marc-André Lureau
> ---
> 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(_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(_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, _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, _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, _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
Re: [Qemu-devel] [PATCH 26/31] tpm: Clean up error reporting in tpm_init_tpmdev()
On 08/10/2018 19:31, Markus Armbruster 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
> Signed-off-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
> ---
> 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(_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(_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, _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, _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, _fatal);
>
> -if (tpm_init() < 0) {
> -exit(1);
> -}
> +tpm_init();
>
> /* init the bluetooth world */
> if (foreach_device_config(DEV_BT, bt_parse))
>
