On 7/23/20 1:58 PM, Markus Armbruster wrote: > This reverts commit d10e05f15d5c3dd5e5cc59c5dfff460d89d48580. > > We report some -tpmdev failures, but then continue as if all was fine. > Reproducer: > > $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -chardev > null,id=tpm0 -tpmdev emulator,id=tpm0,chardev=chrtpm -device > tpm-tis,tpmdev=tpm0 > qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: > tpm-emulator: tpm chardev 'chrtpm' not found. > qemu-system-x86_64: -tpmdev emulator,id=tpm0,chardev=chrtpm: > tpm-emulator: Could not cleanly shutdown the TPM: No such file or directory > QEMU 5.0.90 monitor - type 'help' for more information > (qemu) qemu-system-x86_64: -device tpm-tis,tpmdev=tpm0: Property > 'tpm-tis.tpmdev' can't find value 'tpm0' > $ echo $? > 1 > > This is a regression caused by commit d10e05f15d "tpm: Clean up error > reporting in tpm_init_tpmdev()". It's incomplete: be->create(opts) > continues to use error_report(), and we don't set an error when it > fails. > > I figure converting the create() methods to Error would make some > sense, but I'm not sure it's worth the effort right now. Revert the > broken commit instead, and add a comment to tpm_init_tpmdev(). > > Straightforward conflict in tpm.c resolved. > > Signed-off-by: Markus Armbruster <arm...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> > --- > include/sysemu/tpm.h | 2 +- > softmmu/vl.c | 4 +++- > stubs/tpm.c | 3 ++- > tpm.c | 30 +++++++++++++++++++++--------- > 4 files changed, 27 insertions(+), 12 deletions(-) > > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h > index 03fb25941c..730c61ac97 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); > -void tpm_init(void); > +int tpm_init(void); > void tpm_cleanup(void); > > typedef enum TPMVersion { > diff --git a/softmmu/vl.c b/softmmu/vl.c > index f476ef89ed..2c06cf0513 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -4262,7 +4262,9 @@ void qemu_init(int argc, char **argv, char **envp) > user_creatable_add_opts_foreach, > object_create_delayed, &error_fatal); > > - tpm_init(); > + if (tpm_init() < 0) { > + exit(1); > + } > > blk_mig_init(); > ram_mig_init(); > diff --git a/stubs/tpm.c b/stubs/tpm.c > index 66c99d667d..9bded191d9 100644 > --- a/stubs/tpm.c > +++ b/stubs/tpm.c > @@ -10,8 +10,9 @@ > #include "sysemu/tpm.h" > #include "hw/acpi/tpm.h" > > -void tpm_init(void) > +int tpm_init(void) > { > + return 0; > } > > void tpm_cleanup(void) > diff --git a/tpm.c b/tpm.c > index fe03b24858..f6045bb6da 100644 > --- a/tpm.c > +++ b/tpm.c > @@ -81,26 +81,33 @@ TPMBackend *qemu_find_tpm_be(const char *id) > > static int tpm_init_tpmdev(void *dummy, QemuOpts *opts, Error **errp) > { > + /* > + * Use of error_report() in a function with an Error ** parameter > + * is suspicious. It is okay here. The parameter only exists to > + * make the function usable with qemu_opts_foreach(). It is not > + * actually used. > + */ > const char *value; > const char *id; > const TPMBackendClass *be; > TPMBackend *drv; > + Error *local_err = NULL; > int i; > > if (!QLIST_EMPTY(&tpm_backends)) { > - error_setg(errp, "Only one TPM is allowed."); > + error_report("Only one TPM is allowed."); > return 1; > } > > id = qemu_opts_id(opts); > if (id == NULL) { > - error_setg(errp, QERR_MISSING_PARAMETER, "id"); > + error_report(QERR_MISSING_PARAMETER, "id"); > return 1; > } > > value = qemu_opt_get(opts, "type"); > if (!value) { > - error_setg(errp, QERR_MISSING_PARAMETER, "type"); > + error_report(QERR_MISSING_PARAMETER, "type"); > tpm_display_backend_drivers(); > return 1; > } > @@ -108,14 +115,15 @@ 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_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a TPM backend type"); > + error_report(QERR_INVALID_PARAMETER_VALUE, > + "type", "a TPM backend type"); > tpm_display_backend_drivers(); > return 1; > } > > /* validate backend specific opts */ > - if (!qemu_opts_validate(opts, be->opts, errp)) { > + if (!qemu_opts_validate(opts, be->opts, &local_err)) { > + error_report_err(local_err); > return 1; > } > > @@ -148,10 +156,14 @@ void tpm_cleanup(void) > * Initialize the TPM. Process the tpmdev command line options describing the > * TPM backend. > */ > -void tpm_init(void) > +int tpm_init(void) > { > - qemu_opts_foreach(qemu_find_opts("tpmdev"), > - tpm_init_tpmdev, NULL, &error_fatal); > + if (qemu_opts_foreach(qemu_find_opts("tpmdev"), > + tpm_init_tpmdev, NULL, NULL)) { > + return -1; > + } > + > + return 0; > } > > /* >