Re: [PATCH] tpm_emulator: Report an error if chardev is missing

[Stefan Berger]

On 7/24/20 10:00 AM, Markus Armbruster wrote:
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>
> > This patch fixes the odd error reporting when trying to send a file
> > descriptor to the TPM emulator if one has not passed a valid chardev.
> >
> > $ x86_64-softmmu/qemu-system-x86_64 -tpmdev emulator,id=tpm0
> > qemu-system-x86_64: -tpmdev emulator,id=tpm0: tpm-emulator: Failed to send 
> > CMD_SET_DATAFD: Success
> > qemu-system-x86_64: -tpmdev emulator,id=tpm0: tpm-emulator: Could not cleanly 
> > shutdown the TPM: Success
> >
> > This is the new error report:
> >
> > $ x86_64-softmmu/qemu-system-x86_64 -tpmdev emulator,id=tpm0
> > qemu-system-x86_64: -tpmdev emulator,id=tpm0: tpm-emulator: missing chardev
>                                                  ~~~~~~~~~~~~~~
>
> The "tpm-emulator: " part looks superfluous.
>
> Hmm, many error messages in this file have it.  Feel free to keep it
> now for consistency, and clean it up later.
>
> We usually report missing option parameters like "parameter 'chardev' is
> missing".  Please consider sticking to that.

Fixed.


> > This change does not hide the display of supported TPM types if a non-existent 
> > type is passed:
> >
> > $ x86_64-softmmu/qemu-system-x86_64 -tpmdev nonexistent,id=tpm0
> > qemu-system-x86_64: -tpmdev nonexistent,id=tpm0: Parameter 'type' expects a TPM 
> > backend type
> > Supported TPM types (choose only one):
> >   passthrough   Passthrough TPM backend driver
> >      emulator   TPM emulator backend driver
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >   backends/tpm/tpm_emulator.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> > index 9605339f93..55cbc9c60e 100644
> > --- a/backends/tpm/tpm_emulator.c
> > +++ b/backends/tpm/tpm_emulator.c
> > @@ -568,6 +568,9 @@ static int tpm_emulator_handle_device_opts(TPMEmulator 
> > *tpm_emu, QemuOpts *opts)
> >           }
> >   
> >           tpm_emu->options->chardev = g_strdup(value);
> > +    } else {
> > +        error_report("tpm-emulator: missing chardev");
> > +        goto err;
> >       }
> >   
> >       if (tpm_emulator_prepare_data_fd(tpm_emu) < 0) {
> This is minimally invasive.
>
> I'd prefer
>
>         Error *err = NULL;
>         const char *value;
>         Chardev *dev;
>
>         value = qemu_opt_get(opts, "chardev");
>         if (!value) {
>             error_report("tpm-emulator: missing chardev");
>             goto err;
>         }
>
>         dev = qemu_chr_find(value);
>         if (!dev) {
>             error_report("tpm-emulator: tpm chardev '%s' not found.", value);
>             goto err;
>         }
>
> Your choice.


Ok, changed it to this.


> > @@ -925,6 +928,11 @@ static void tpm_emulator_shutdown(TPMEmulator *tpm_emu)
> >   {
> >       ptm_res res;
> >   
> > +    if (!tpm_emu->options->chardev) {
> > +        /* was never properly initialized */
> > +        return;
> > +    }
> > +
> Is this still reachable?  If yes, how?


tpm_emulator_inst_finalize still runs.


Sending V2. Thanks.

> If not, drop the hunk and have my
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> >       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SHUTDOWN, &res, 0, sizeof(res)) < 
> > 0) {
> >           error_report("tpm-emulator: Could not cleanly shutdown the TPM: %s",
> >                        strerror(errno));