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

[Markus Armbruster]

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.

>
> 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.

> @@ -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?

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