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

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

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 <amarnath.valluri@intel.com>
>> ---
>>   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