Re: [Qemu-devel] [PATCH] tpm: Fix compilation with --disable-tpm

["Valluri, Amarnath" <amarnath.valluri@intel.com>]

Sorry for causing build break :(

I would prefer the way tpm_init() was handled in vl.c, even
tpm_cleanup() shuould be guarded behind #ifdef CONFIG_TPM.

- Amarnath
On Wed, 2017-10-18 at 11:27 +0200, Juan Quintela wrote:
> Laurent Vivier <lvivier@redhat.com> wrote:
> > 
> > On 18/10/2017 10:33, Juan Quintela wrote:
> > > 
> > > Commit
> > >    c37cacabf2285b0731b44c1f667781fdd4f2b658
> > > 
> > > broke compilation without tpm.  Just add an empty tpm_cleanup()
> > > stub.
> > tpm_init() and tpm_config_parse() are managed in a different way:
> > they
> > are included in a "#ifdef CONFIG_TPM .. #endif" in vl.c
> > 
> > I think you should follow the same way.
> tpm is weird (TM):
> 
> in include/sysemu/tpm.h we do that in an incline function
> 
> static inline TPMVersion tpm_get_version(void)
> {
> #ifdef CONFIG_TPM
>     Object *obj = object_resolve_path_type("", TYPE_TPM_TIS, NULL);
> 
>     if (obj) {
>         return tpm_tis_get_tpm_version(obj);
>     }
> #endif
>     return TPM_VERSION_UNSPEC;
> }
> 
> 
> tpm.c, we have an ifdef in the middle of the file
> 
> #ifdef CONFIG_TPM
> 
> #endif
> 
> 
> vl.c, we protect tpm_* calls with CONFIG_TPM
> 
> #ifdef CONFIG_TPM
>             case QEMU_OPTION_tpmdev:
>                 if (tpm_config_parse(qemu_find_opts("tpmdev"),
> optarg) < 0) {
>                     exit(1);
>                 }
>                 break;
> #endif
> 
> 
> but we almost never do:
> 
> #ifdef CONFIG_TPM
>    tpm_cleanup()
> #endif
> 
> We normally create an stub function.
> 
> So ......
> 
> We are going to be inconsistent whatever we did.
> 
> I would have vote for
> 
> #ifdef CONFIG_TPM
> void tpm_cleanup(void);
> #else
> static inline void tpm_cleanup(void) {}
> #endif
> 
> On the header file (it was my first solution), but having CONFIG_TPM
> on
> tpm.c sounded gross.
> 
> So ....
> 
> I think that now that the problem is spotted, I will left the choose
> of
> the solution to the maintaner O:-)
> 
> Later, Juan.