Re: [Qemu-devel] [PATCH V19 1/7] Support for TPM command line options

[Corey Bryant]

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