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

[Luiz Capitulino]

On Fri, 01 Feb 2013 10:33:01 -0500
Corey Bryant <address@hidden> wrote:

> > +##
> > +# @TPMInfo:
> > +#
> > +# Information about the TPM
> > +#
> > +# @model: The TPM frontend model, i.e., tpm-tis
> > +#
> > +# @id: The ID of the TPM
> > +#
> > +# @type: The type of TPM backend, i.e., passthrough
> > +#
> > +# @path: #optional Path to the TPM backend device
> > +#
> > +# @cancel_path: #optional Path to TPM backend device's cancel sysfs entry
> > +#
> > +# Since: 1.5.0
> > +##
> > +{ 'type': 'TPMInfo',
> > +  'data': {'model': 'str', 'id': 'str', 'type': 'str', '*path': 'str',
> > +           '*cancel_path': 'str' } }
> > +
> 
> It might be preferred that you break the monitor support into it's own 
> patch, and logically it would make more sense to introduce these backend 
> specific members after the backend patch.  But those are just nit comments.
> 
> More importantly, I have a question (probably for Luiz) regarding future 
> modification of the monitor command.  In the future, if a new vTPM 
> backend is introduced (e.g. an emulated software vTPM), can we add new 
> optional members to the end of this TPMInfo?  For example, could we 
> modify it to this in the future?

Usually, extensions like that are only allowed for query- commands. For
non-query commands we prefer adding new commands instead.

Btw, we avoid using free strings like 'type' and 'model', please use
an enumeration instead.

PS: I wonder if it would be possible to extend enumerations and unions
    though.