Re: [Qemu-devel] [PATCH 5/7] tmp backend: Add new api to read backend tpm options

[Eric Blake <eblake@redhat.com>]

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

On 03/31/2017 08:10 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 tpm backend api, tpm_backend_get_tpm_options()
> to read the backend configured options.
> 
> Added new method, get_tpm_options() to TPMDriverOps., which shall be implemented
> by the derived classes to return configured tpm options.
> 
> Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
> ---

Just an interface review for now:


> +++ b/qapi-schema.json
> @@ -5140,6 +5140,14 @@
>  { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>  
>  ##
> +# @TPMOptions:
> +#
> +# Base type for TPM options
> +##

Missing a 'Since: 2.10' designation

> +{ 'struct': 'TPMOptions',
> +  'data': { } }

This class feels pointless (at least for now; I haven't checked if you
expand it later in the series - but if so, define it where you first
need it). It has no members, and you are only using it...

> +
> +##
>  # @TPMPassthroughOptions:
>  #
>  # Information about the TPM passthrough type
> @@ -5151,8 +5159,8 @@
>  #
>  # Since: 1.5
>  ##
> -{ 'struct': 'TPMPassthroughOptions', 'data': { '*path' : 'str',
> -                                             '*cancel-path' : 'str'} }
> +{ 'struct': 'TPMPassthroughOptions', 'base': 'TPMOptions',

...as a base class. A base class makes sense only if it is consolidating
common members, or if it is the base to at least two different
structures where having a common C type to pass around is handy.

> +  'data': { '*path' : 'str', '*cancel-path' : 'str'} }
>  

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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