Move TPM passthrough specific command line options to backend structure

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 <stefanb@linux.vnet.ibm.com>

---
 tpm/tpm.c             |   10 +++++++++-
 tpm/tpm_int.h         |    8 ++++++++
 tpm/tpm_passthrough.c |   16 ++++++++++++++++
 vl.c                  |   15 ---------------
 4 files changed, 33 insertions(+), 16 deletions(-)

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 <stefanb@linux.vnet.ibm.com>
>
> ---
>   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;
>
>

On 04/10/2013 02:51 PM, Corey Bryant wrote:
>
>
> On 04/02/2013 01:30 PM, Stefan Berger wrote:
>>           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.

Well, the function does what is needed here, sorting out command line 
parameters that are unknown. It also fits the available parameters.

I fixed the code related to the other comments. Thanks for the review.

    Stefan
I