From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47662) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIJGC-0002wA-4C for qemu-devel@nongnu.org; Wed, 20 Mar 2013 09:40:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UIJGA-0006KM-3K for qemu-devel@nongnu.org; Wed, 20 Mar 2013 09:40:55 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:58036) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIJ7E-0002FJ-Fo for qemu-devel@nongnu.org; Wed, 20 Mar 2013 09:31:40 -0400 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 20 Mar 2013 07:31:34 -0600 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id F35653E4004E for ; Wed, 20 Mar 2013 07:26:40 -0600 (MDT) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2KDQmfc186736 for ; Wed, 20 Mar 2013 07:26:48 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2KDQlDi030932 for ; Wed, 20 Mar 2013 07:26:47 -0600 Message-ID: <5149B915.2060808@linux.vnet.ibm.com> Date: Wed, 20 Mar 2013 09:26:45 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1363367835-11306-1-git-send-email-coreyb@linux.vnet.ibm.com> <878v5k65ur.fsf@blackfin.pond.sub.org> <51475310.2060008@linux.vnet.ibm.com> <87d2uvu9yk.fsf@blackfin.pond.sub.org> <51487D66.8010007@linux.vnet.ibm.com> <87wqt2jlol.fsf@blackfin.pond.sub.org> In-Reply-To: <87wqt2jlol.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] QMP: TPM QMP and man page documentation updates List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Anthony Liguori , lcapitulino@redhat.com, qemu-devel@nongnu.org, Stefan Berger On 03/20/2013 08:32 AM, Markus Armbruster wrote: > Corey Bryant writes: > >> On 03/19/2013 03:26 AM, Markus Armbruster wrote: >>> [Note cc: Anthony for QAPI schema expertise] >>> >>> Stefan Berger writes: >>> >>>> On 03/18/2013 12:16 PM, Markus Armbruster wrote: >>>>> Corey Bryant writes: >>>>> >>>>>> Signed-off-by: Corey Bryant >>>>>> --- >>>>>> 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. > > Yes, please! > Will do. >>> 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. > > A union in the schema implicitely defines an C enumeration type to be > used for its discriminator. For instance, union TpmTypeOptions > implicitely defines this one: > > typedef enum TpmTypeOptionsKind > { > TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH_OPTIONS = 0, > TPM_TYPE_OPTIONS_KIND_MAX = 1, > } TpmTypeOptionsKind; > > QAPI's discriminated union becomes a C struct containing the > discriminator and the union of the members: > > struct TpmTypeOptions > { > TpmTypeOptionsKind kind; > union { > void *data; > TPMPassthroughOptions * tpm_passthrough_options; > }; > }; > > Enum TpmType and type TpmInfo become: > > typedef enum TpmType > { > TPM_TYPE_PASSTHROUGH = 0, > TPM_TYPE_MAX = 1, > } TpmType; > > struct TPMInfo > { > char * id; > TpmModel model; > TpmType type; > TpmTypeOptions * tpm_options; > }; > > With the change I propose, TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH_OPTIONS > becomes TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH, and TPMInfo member type > goes away. > > Because TpmType is still used elsewhere, it doesn't go away, thus still > duplicates TpmTypeOptionsKind. My question to Anthony was about ways to > avoid that remaining bit of duplication. I don't expect you do answer > it :) > Ok well thanks for the explanation anyway! -- Regards, Corey Bryant