From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49278) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TWUS9-0007k2-8S for qemu-devel@nongnu.org; Thu, 08 Nov 2012 10:55:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TWUS7-00050z-P0 for qemu-devel@nongnu.org; Thu, 08 Nov 2012 10:55:37 -0500 Received: from e7.ny.us.ibm.com ([32.97.182.137]:46376) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TWUS7-00050M-Jt for qemu-devel@nongnu.org; Thu, 08 Nov 2012 10:55:35 -0500 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 8 Nov 2012 10:55:29 -0500 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 9AF2A38C8096 for ; Thu, 8 Nov 2012 10:52:54 -0500 (EST) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qA8FqsYm324992 for ; Thu, 8 Nov 2012 10:52:54 -0500 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 qA8FqrQq002783 for ; Thu, 8 Nov 2012 13:52:54 -0200 Message-ID: <509BD554.1080708@linux.vnet.ibm.com> Date: Thu, 08 Nov 2012 10:52:52 -0500 From: Corey Bryant MIME-Version: 1.0 References: <1338838668-7544-1-git-send-email-stefanb@linux.vnet.ibm.com> <1338838668-7544-2-git-send-email-stefanb@linux.vnet.ibm.com> <50645EDF.8060105@linux.vnet.ibm.com> <50883C48.5090607@linux.vnet.ibm.com> In-Reply-To: <50883C48.5090607@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V19 1/7] Support for TPM command line options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger Cc: andreas.niederl@iaik.tugraz.at, qemu-devel@nongnu.org, anthony@codemonkey.ws, mst@redhat.com On 10/24/2012 03:06 PM, Stefan Berger wrote: > On 09/27/2012 10:12 AM, Corey Bryant wrote: >> >> >> On 06/04/2012 03:37 PM, Stefan Berger wrote: >>> + >>> +#ifdef CONFIG_TPM >>> + >>> +static const TPMDriverOps *bes[] = { >> >> I think bes[] would be more descriptive if it were named be_drivers[] >> or be_ops[]? >> > > Renamed to be_drivers. > >>> + if (!QLIST_EMPTY(&tpm_backends)) { >>> + error_report("Only one TPM is allowed.\n"); >>> + return 1; >>> + } >> >> A list of tpm_backends is maintained and walked in a few places, but >> only one is allowed to be added to the list. Will it ever make sense >> to enable multiple backends at one time? >> > > A list is also returned through the monitor. This list can at the moment > only have maximum of one entry. I would keep that list there unless > someone else opposes. It may be possible to create different types of > hardware emulation interfaces or simply replicate the TPM TIS at > different addresses. So I cannot say whether it will 'ever make sense' > to do that but I'd rather keep the opportunity there than close it and > with that also let the monitor return a list of items rather than a > single item. > > I removed the processing of the lists in this part of the code at least. > Ok and it doesn't hurt to keep the list processing. In that case you might as well keep the list processing code everywhere that you already have it. >>> + >>> + value = qemu_opt_get(opts, "type"); >>> + if (!value) { >>> + qerror_report(QERR_MISSING_PARAMETER, "type"); >>> + tpm_display_backend_drivers(); >>> + return 1; >>> + } >>> + >>> + be = tpm_get_backend_driver(value); >> >> The "type" value is being passed but the tpm_get_backend_driver() >> defines the parameter as "id". Maybe "id" could be renamed to "type" >> for consistency. See similar comment further down in this email. >> > > Done. > >>> + */ >>> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg) >>> +{ >>> + QemuOpts *opts; >>> + >>> + if (strcmp("none", optarg) != 0) { >> >> What's the point of supporting "-tpmdev none"? >> > > Removed. > There must have been a reason you added it in the first place that I'm just not aware of. Did someone else suggest adding it? >>> +typedef struct TPMBackend { >>> + char *id; >> >> For consistency, this could be named "type" instead of "id" since it >> corresponds to -tpmdev's type. >> > > Yes. > >>> + uint8_t command_locty; >> >> It would be easier to read if locality was spelled out fully, instead >> of locty. But if that causes lines to be too long then maybe it's not >> worth it. > > I rather keep it 'locty'. > >> >>> + TPMLocality *cmd_locty; >> >> There's a cmd_locty and a command_locty. command_locty is the locality >> number and cmd_locty is the locality data. Could these variable names >> be updated to be more unique and descriptive? >> > > Will rename them command_locty -> locty_number and cmd_locty -> locty_data. > > Regards, > > Stefan > > -- Regards, Corey Bryant