Re: [Qemu-devel] [PATCH] Move TPM passthrough specific command line options to backend structure

[Corey Bryant]

On 04/02/2013 01:30 PM, Stefan Berger wrote:
> Move the TPM passthrough specific command line options to the passthrough
> backend implementation and attach them to the backend's interface structure.
>
> Add code to tpm.c for validating the TPM command line options.
>
> Signed-off-by: Stefan Berger <address@hidden>
>
> ---
>   tpm/tpm.c             |   10 +++++++++-
>   tpm/tpm_int.h         |    8 ++++++++
>   tpm/tpm_passthrough.c |   16 ++++++++++++++++
>   vl.c                  |   15 ---------------
>   4 files changed, 33 insertions(+), 16 deletions(-)
>
> Index: qemu-git.pt/vl.c
> ===================================================================
> --- qemu-git.pt.orig/vl.c
> +++ qemu-git.pt/vl.c
> @@ -502,21 +502,6 @@ static QemuOptsList qemu_tpmdev_opts = {
>       .implied_opt_name = "type",
>       .head = QTAILQ_HEAD_INITIALIZER(qemu_tpmdev_opts.head),
>       .desc = {
> -        {
> -            .name = "type",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Type of TPM backend",
> -        },
> -        {
> -            .name = "cancel-path",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Sysfs file entry for canceling TPM commands",
> -        },
> -        {
> -            .name = "path",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Path to TPM device on the host",
> -        },
>           { /* end of list */ }

Does this need a comment to say where these options are now defined?

>       },
>   };
> Index: qemu-git.pt/tpm/tpm.c
> ===================================================================
> --- qemu-git.pt.orig/tpm/tpm.c
> +++ qemu-git.pt/tpm/tpm.c
> @@ -146,7 +146,7 @@ static int configure_tpm(QemuOpts *opts)
>       const char *id;
>       const TPMDriverOps *be;
>       TPMBackend *drv;
> -    Error *local_err = NULL;
> +    Error *local_err = NULL, *errp = NULL;

Can't you use local_err in the new code below instead of creating a new 
variable?

>       if (!QLIST_EMPTY(&tpm_backends)) {
>           error_report("Only one TPM is allowed.\n");
> @@ -174,6 +174,14 @@ static int configure_tpm(QemuOpts *opts)
>           return 1;
>       }
>
> +    /* validate backend specific opts */
> +    qemu_opts_validate(opts, be->opts, &errp);
> +    if (error_is_set(&errp)) {
> +        qerror_report_err(errp);
> +        error_free(errp);
> +        return 1;
> +    }
> +

This looks fine to me but I see this is the first call to 
qemu_opts_validate() in QEMU.  I wonder why.

>       drv = be->create(opts, id);
>       if (!drv) {
>           return 1;
> Index: qemu-git.pt/tpm/tpm_passthrough.c
> ===================================================================
> --- qemu-git.pt.orig/tpm/tpm_passthrough.c
> +++ qemu-git.pt/tpm/tpm_passthrough.c
> @@ -512,8 +512,24 @@ static void tpm_passthrough_destroy(TPMB
>       g_free(tpm_pt->tpm_dev);
>   }
>
> +static const QemuOptDesc tpm_passthrough_cmdline_opts[] = {
> +    TPM_STANDARD_CMDLINE_OPTS,
> +    {
> +        .name = "cancel-path",
> +        .type = QEMU_OPT_STRING,
> +        .help = "Sysfs file entry for canceling TPM commands",
> +    },
> +    {
> +        .name = "path",
> +        .type = QEMU_OPT_STRING,
> +        .help = "Path to TPM device on the host",
> +    },
> +    { /* end of list */ },
> +};
> +
>   const TPMDriverOps tpm_passthrough_driver = {
>       .type                     = TPM_TYPE_PASSTHROUGH,
> +    .opts                     = tpm_passthrough_cmdline_opts,
>       .desc                     = tpm_passthrough_create_desc,
>       .create                   = tpm_passthrough_create,
>       .destroy                  = tpm_passthrough_destroy,
> Index: qemu-git.pt/tpm/tpm_int.h
> ===================================================================
> --- qemu-git.pt.orig/tpm/tpm_int.h
> +++ qemu-git.pt/tpm/tpm_int.h
> @@ -40,6 +40,7 @@ typedef void (TPMRecvDataCB)(TPMState *,
>
>   struct TPMDriverOps {
>       enum TpmType type;
> +    const QemuOptDesc *opts;
>       /* get a descriptive text of the backend to display to the user */
>       const char *(*desc)(void);
>
> @@ -64,6 +65,13 @@ struct TPMDriverOps {
>       bool (*get_tpm_established_flag)(TPMBackend *t);
>   };
>
> +#define TPM_STANDARD_CMDLINE_OPTS \
> +    { \
> +        .name = "type", \
> +        .type = QEMU_OPT_STRING, \
> +        .help = "Type of TPM backend", \
> +    }
> +
>   struct tpm_req_hdr {
>       uint16_t tag;
>       uint32_t len;

--
Regards,
Corey Bryant