From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35718) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQ07Z-00064f-7k for qemu-devel@nongnu.org; Wed, 10 Apr 2013 14:51:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UQ07X-00022t-4i for qemu-devel@nongnu.org; Wed, 10 Apr 2013 14:51:49 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:37846) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQ07X-00022T-0X for qemu-devel@nongnu.org; Wed, 10 Apr 2013 14:51:47 -0400 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 10 Apr 2013 14:51:45 -0400 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 37AC338C801A for ; Wed, 10 Apr 2013 14:51:33 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3AIpUwF48496810 for ; Wed, 10 Apr 2013 14:51:30 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3AIpUvW018828 for ; Wed, 10 Apr 2013 15:51:30 -0300 Message-ID: <5165B4B1.2020003@linux.vnet.ibm.com> Date: Wed, 10 Apr 2013 14:51:29 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1364923843.339.2.camel@d941e-10> In-Reply-To: <1364923843.339.2.camel@d941e-10> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Move TPM passthrough specific command line options to backend structure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger Cc: "qemu-devel@nongnu.org" 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 > > --- > 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