Re: [Qemu-devel] [PATCH] QMP: TPM QMP and man page documentation updates

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 03/19/2013 03:26 AM, Markus Armbruster wrote:
> [Note cc: Anthony for QAPI schema expertise]
>
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>
>> On 03/18/2013 12:16 PM, Markus Armbruster wrote:
>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>
>>>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>>>> ---
>>>>    qemu-options.hx |  3 ++-
>>>>    qmp-commands.hx | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 61 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>> index 30fb85d..3b3cd0f 100644
>>>> --- a/qemu-options.hx
>>>> +++ b/qemu-options.hx
>>>> @@ -2237,7 +2237,8 @@ Backend type must be:
>>>>    @option{passthrough}.
>>>>      The specific backend type will determine the applicable
>>>> options.
>>>> -The @code{-tpmdev} option requires a @code{-device} option.
>>>> +The @code{-tpmdev} option creates the TPM backend and requires a
>>>> +@code{-device} option that specifies the TPM frontend interface model.
>>>>      Options to each backend are described below.
>>>>    diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index b370060..4eda5ea 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -2721,18 +2721,77 @@ EQMP
>>>>            .mhandler.cmd_new = qmp_marshal_input_query_tpm,
>>>>        },
>>>>    +SQMP
>>>> +query-tpm
>>>> +---------
>>>> +
>>>> +Return information about the TPM device.
>>>> +
>>>> +Arguments: None
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "query-tpm" }
>>>> +<- { "return":
>>>> +     [
>>>> +       { "model": "tpm-tis",
>>>> +         "tpm-options":
>>>> +           { "type": "tpm-passthrough-options",
>>>> +             "data":
>>>> +               { "cancel-path": "/sys/class/misc/tpm0/device/cancel",
>>>> +                 "path": "/dev/tpm0"
>>>> +               }
>>>> +           },
>>>> +         "type": "passthrough",
>>>> +         "id": "tpm0"
>>>> +       }
>>>> +     ]
>>>> +   }
>>>> +
>>>> +EQMP
>>>> +
>>> "tpm-options" is a discriminated union.  How is its discriminator "type"
>>> (here: "tpm-passthrough-options") related to the outer "type" (here:
>>> "passthrough")?
>>
>> It gives you similar information twice. So there is a direct
>> relationship between the two types.
>
> Awkward and undocumented.
>
> Relevant parts of qapi-schema.json:
>
>      { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
>
>      { 'union': 'TpmTypeOptions',
>         'data': { 'tpm-passthrough-options' : 'TPMPassthroughOptions' } }
>
>      { 'type': 'TPMInfo',
>        'data': {'id': 'str',
>                 'model': 'TpmModel',
>                 'type': 'TpmType',
>                 'tpm-options': 'TpmTypeOptions' } }
>
> Type Netdev solves the same problem more elegantly:
>
>      { 'union': 'NetClientOptions',
>        'data': {
>          'none':     'NetdevNoneOptions',
>          'nic':      'NetLegacyNicOptions',
>          'user':     'NetdevUserOptions',
>          'tap':      'NetdevTapOptions',
>          'socket':   'NetdevSocketOptions',
>          'vde':      'NetdevVdeOptions',
>          'dump':     'NetdevDumpOptions',
>          'bridge':   'NetdevBridgeOptions',
>          'hubport':  'NetdevHubPortOptions' } }
>
>      { 'type': 'Netdev',
>        'data': {
>          'id':   'str',
>          'opts': 'NetClientOptions' } }
>
> Uses the union's discriminator.  Straightforward.
>
> Following Netdev precedence, we get:
>
>      { 'union': 'TpmTypeOptions',
>        'data': { 'passthrough' : 'TPMPassthroughOptions' } }
>
>      { 'type': 'TPMInfo',
>        'data': {'id': 'str',
>                 'model': 'TpmModel',
>                 'opts': 'TpmTypeOptions' } }
>

I can send a patch for this update if you'd like.

> Duplication of type is gone.  No need for documentation.
>
> Since enum TpmType is used elsewhere, it still gets duplicated in the
> union's discriminator.  Anthony, is there a way to name the implicit
> discriminator enum type for reference elsewhere in the schema?
>

Are you saying it still gets duplicated with this fix?  I'm not sure 
what you mean.

-- 
Regards,
Corey Bryant