Re: [Qemu-devel] [PATCH v3 5/8] tmp backend: Add new api to read backend TpmInfo

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]

On 05/02/2017 06:52 AM, Amarnath Valluri wrote:
> TPM configuration options are backend implementation details and shall not be
> part of base TPMBackend object, and these shall not be accessed directly outside
> of the class, hence added a new interface method, get_tpm_options() to
> TPMDriverOps., which shall be implemented by the derived classes to return
> configured tpm options.
> 
> A new tpm backend api - tpm_backend_query_tpm() which uses _get_tpm_options() to
> prepare TpmInfo.
> 
> Made TPMPassthroughOptions type inherited from newly defined TPMOptions base type.
> The TPMOptions base type is intentionally left empty and supposed to be
> inherited by all backend implementations to define their tpm configuration
> options.
> 
> Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
> ---

> +++ b/qapi-schema.json
> @@ -5137,6 +5137,16 @@
>  { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>  
>  ##
> +# @TPMOptions:
> +#
> +# Base type for TPM options
> +#
> +# Since: 2.10
> +##
> +{ 'struct': 'TPMOptions',
> +  'data': { } }
> +
> +##
>  # @TPMPassthroughOptions:
>  #
>  # Information about the TPM passthrough type
> @@ -5148,20 +5158,9 @@
>  #
>  # Since: 1.5
>  ##
> -{ 'struct': 'TPMPassthroughOptions', 'data': { '*path' : 'str',
> -                                             '*cancel-path' : 'str'} }
> +{ 'struct': 'TPMPassthroughOptions', 'base': 'TPMOptions',
> +  'data': { '*path' : 'str', '*cancel-path' : 'str'} }
>  
> -##
> -# @TpmTypeOptions:
> -#
> -# A union referencing different TPM backend types' configuration options
> -#
> -# @type: 'passthrough' The configuration options for the TPM passthrough type
> -#
> -# Since: 1.5
> -##
> -{ 'union': 'TpmTypeOptions',
> -   'data': { 'passthrough' : 'TPMPassthroughOptions' } }

Getting rid of a simple union is nice.  However,

>  
>  ##
>  # @TPMInfo:
> @@ -5170,6 +5169,8 @@
>  #
>  # @id: The Id of the TPM
>  #
> +# @type: The TPM backend type
> +#
>  # @model: The TPM frontend model
>  #
>  # @options: The TPM (backend) type configuration options
> @@ -5178,8 +5179,9 @@
>  ##
>  { 'struct': 'TPMInfo',
>    'data': {'id': 'str',
> +           'type': 'TpmType',
>             'model': 'TpmModel',
> -           'options': 'TpmTypeOptions' } }
> +           'options': 'TPMOptions' } }
>  
>  ##
>  # @query-tpm:
> @@ -5196,13 +5198,12 @@
>  # <- { "return":
>  #      [
>  #        { "model": "tpm-tis",
> +#          "type": "passthrough",
>  #          "options":
> -#            { "type": "passthrough",
> -#              "data":
> -#                { "cancel-path": "/sys/class/misc/tpm0/device/cancel",
> -#                  "path": "/dev/tpm0"
> -#                }
> -#            },
> +#          {
> +#            "cancel-path": "/sys/class/misc/tpm0/device/cancel",
> +#            "path": "/dev/tpm0"
> +#          },
>  #          "id": "tpm0"

Doing it in a manner that is NOT backwards-compatible (as evidenced by
your change to the output text) is NOT good.  It will break any existing
client that was expecting to parse information in the old layout.

I'm afraid you're going to have to fix this to not break QMP wire
compatibility.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]