Re: [Qemu-devel] [PATCH v2 6/9] tpm-backend: Remove unneeded destroy() method from TpmDriverOps interface

[Amarnath Valluri]

On 04/25/2017 09:59 PM, Stefan Berger wrote:
> On 04/07/2017 10:30 AM, Amarnath Valluri wrote:
> > As TPMBackend is a Qemu Object, we can use object_unref() inplace of
> > tpm_backend_destroy() to free the backend object, hence removed 
> > destroy() from
> > TPMDriverOps interface.
> >
> > Signed-off-by: Amarnath Valluri <address@hidden>
> > ---
> >   backends/tpm.c               | 11 -----------
> >   hw/tpm/tpm_passthrough.c     | 14 --------------
> >   include/sysemu/tpm_backend.h |  7 -------
> >   tpm.c                        |  2 +-
> >   4 files changed, 1 insertion(+), 33 deletions(-)
> >
> > diff --git a/backends/tpm.c b/backends/tpm.c
> > index c96f462..3493df6 100644
> > --- a/backends/tpm.c
> > +++ b/backends/tpm.c
> > @@ -51,17 +51,6 @@ const char *tpm_backend_get_desc(TPMBackend *s)
> >       return k->ops->desc ? k->ops->desc() : "";
> >   }
> >
> > -void tpm_backend_destroy(TPMBackend *s)
> > -{
> > -    TPMBackendClass *k = TPM_BACKEND_GET_CLASS(s);
> > -
> > -    if (k->ops->destroy) {
> > -        k->ops->destroy(s);
> > -    }
> > -
> > -    object_unref(OBJECT(s));
> > -}
> > -
> >   int tpm_backend_init(TPMBackend *s, TPMState *state,
> >                        TPMRecvDataCB *datacb)
> >   {
> > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> > index 71bdf25..8e11ed3 100644
> > --- a/hw/tpm/tpm_passthrough.c
> > +++ b/hw/tpm/tpm_passthrough.c
> > @@ -428,19 +428,6 @@ err_exit:
> >       return NULL;
> >   }
> >
> > -static void tpm_passthrough_destroy(TPMBackend *tb)
> > -{
> > -    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> > -
> > -    tpm_passthrough_cancel_cmd(tb);
> > -
> > -    qemu_close(tpm_pt->tpm_fd);
> > -    qemu_close(tpm_pt->cancel_fd);
> > -    g_free(tpm_pt->tpm_dev);
>
> This g_free should be in the finalizer as well, right? It isn't. So we 
> will leak some memory.
Yes, I will fix this.
> > -
> > -    qapi_free_TPMPassthroughOptions(tpm_pt->ops);
> > -}
> > -
> >   static TPMOptions *tpm_passthrough_get_tpm_options(TPMBackend *tb)
> >   {
> >       TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> > @@ -483,7 +470,6 @@ static const TPMDriverOps tpm_passthrough_driver = {
> >       .opts                     = tpm_passthrough_cmdline_opts,
> >       .desc                     = tpm_passthrough_create_desc,
> >       .create                   = tpm_passthrough_create,
> > -    .destroy                  = tpm_passthrough_destroy,
> >       .realloc_buffer           = tpm_passthrough_realloc_buffer,
> >       .reset                    = tpm_passthrough_reset,
> >       .had_startup_error        = tpm_passthrough_get_startup_error,
> > diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> > index 7f4d621..8f8f133 100644
> > --- a/include/sysemu/tpm_backend.h
> > +++ b/include/sysemu/tpm_backend.h
> > @@ -77,7 +77,6 @@ struct TPMDriverOps {
> >       const char *(*desc)(void);
> >
> >       TPMBackend *(*create)(QemuOpts *opts, const char *id);
> > -    void (*destroy)(TPMBackend *t);
> >
> >       /* initialize the backend */
> >       int (*init)(TPMBackend *t);
> > @@ -119,12 +118,6 @@ enum TpmType tpm_backend_get_type(TPMBackend *s);
> >   const char *tpm_backend_get_desc(TPMBackend *s);
> >
> >   /**
> > - * tpm_backend_destroy:
> > - * @s: the backend to destroy
> > - */
> > -void tpm_backend_destroy(TPMBackend *s);
> > -
> > -/**
> >    * tpm_backend_init:
> >    * @s: the backend to initialized
> >    * @state: TPMState
> > diff --git a/tpm.c b/tpm.c
> > index 1b6b550..43d980e 100644
> > --- a/tpm.c
> > +++ b/tpm.c
> > @@ -197,7 +197,7 @@ void tpm_cleanup(void)
> >
> >       QLIST_FOREACH_SAFE(drv, &tpm_backends, list, next) {
> >           QLIST_REMOVE(drv, list);
> > -        tpm_backend_destroy(drv);
> > +        object_unref(OBJECT(drv));
> >       }
> >   }
>
> I *think* you should apply this patch after introducing the 
> tpm_passthrough_inst_finalize or merge it into that one.
Ok, i even think it makes sense to meld this commit with 
baeb9df(tpm-backend: Initialize and free data members in it's own methods).

- Amarnath