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

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

[Juan Quintela]

Commit
   c37cacabf2285b0731b44c1f667781fdd4f2b658

broke compilation without tpm.  Just add an empty tpm_cleanup() stub.

CC: Amarnath Valluri <amarnath.valluri@intel.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 tpm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tpm.c b/tpm.c
index 3122227156..9396bb669c 100644
--- a/tpm.c
+++ b/tpm.c
@@ -194,6 +194,12 @@ int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
     return 0;
 }
 
+#else
+
+void tpm_cleanup(void)
+{
+}
+
 #endif /* CONFIG_TPM */
 
 static const TPMDriverOps *tpm_driver_find_by_type(enum TpmType type)
-- 
2.13.6

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

[Laurent Vivier]

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.

Thanks,
Laurent

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

[Juan Quintela]

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.

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

[Valluri, Amarnath]

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.