diff mbox

[v2,3/9] tpm-backend: Initialize and free data members in it's own methods

Message ID 1491575431-32170-4-git-send-email-amarnath.valluri@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Amarnath Valluri April 7, 2017, 2:30 p.m. UTC 
Initialize and free TPMBackend data members in it's own instance_init() and
instance_finalize methods.

Took the opportunity to fix object cleanup in tpm_backend_{create,destroy}
methods

Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
---
 backends/tpm.c           |  8 ++++++--
 hw/tpm/tpm_passthrough.c | 18 +++++++++++-------
 2 files changed, 17 insertions(+), 9 deletions(-)

Comments

Stefan Berger April 25, 2017, 6:27 p.m. UTC | #1 
On 04/07/2017 10:30 AM, Amarnath Valluri wrote:
> Initialize and free TPMBackend data members in it's own instance_init() and
> instance_finalize methods.
>
> Took the opportunity to fix object cleanup in tpm_backend_{create,destroy}
> methods
>
> Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
> ---
>   backends/tpm.c           |  8 ++++++--
>   hw/tpm/tpm_passthrough.c | 18 +++++++++++-------
>   2 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/backends/tpm.c b/backends/tpm.c
> index 0ec0c67..99d2b2b 100644
> --- a/backends/tpm.c
> +++ b/backends/tpm.c
> @@ -57,7 +57,7 @@ void tpm_backend_destroy(TPMBackend *s)
>
>       k->ops->destroy(s);
>
> -    tpm_backend_thread_end(s);
> +    object_unref(OBJECT(s));
>   }
>
>   int tpm_backend_init(TPMBackend *s, TPMState *state,
> @@ -182,11 +182,13 @@ static void tpm_backend_prop_set_opened(Object *obj, bool value, Error **errp)
>
>   static void tpm_backend_instance_init(Object *obj)
>   {
> +    TPMBackend *s = TPM_BACKEND(obj);
> +
>       object_property_add_bool(obj, "opened",
>                                tpm_backend_prop_get_opened,
>                                tpm_backend_prop_set_opened,
>                                NULL);
> -
> +    s->fe_model = -1;
>   }
>
>   static void tpm_backend_instance_finalize(Object *obj)
> @@ -194,6 +196,8 @@ static void tpm_backend_instance_finalize(Object *obj)
>       TPMBackend *s = TPM_BACKEND(obj);
>
>       g_free(s->id);
> +    g_free(s->path);
> +    g_free(s->cancel_path);
>       tpm_backend_thread_end(s);
>   }
>
> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> index f50d9cf..65831b5 100644
> --- a/hw/tpm/tpm_passthrough.c
> +++ b/hw/tpm/tpm_passthrough.c
> @@ -417,8 +417,6 @@ static TPMBackend *tpm_passthrough_create(QemuOpts *opts, const char *id)
>       TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
>
>       tb->id = g_strdup(id);
> -    /* let frontend set the fe_model to proper value */
> -    tb->fe_model = -1;
>
>       if (tpm_passthrough_handle_device_opts(opts, tb)) {
>           goto err_exit;
> @@ -432,7 +430,7 @@ static TPMBackend *tpm_passthrough_create(QemuOpts *opts, const char *id)
>       return tb;
>
>   err_exit:
> -    g_free(tb->id);
> +    object_unref(obj);
>
>       return NULL;
>   }
> @@ -445,10 +443,6 @@ static void tpm_passthrough_destroy(TPMBackend *tb)
>
>       qemu_close(tpm_pt->tpm_fd);
>       qemu_close(tpm_pt->cancel_fd);
> -
> -    g_free(tb->id);
> -    g_free(tb->path);
> -    g_free(tb->cancel_path);


Looks like here you are correcting the error from the previous patch...



>       g_free(tpm_pt->tpm_dev);
>   }
>
> @@ -486,10 +480,20 @@ static const TPMDriverOps tpm_passthrough_driver = {
>
>   static void tpm_passthrough_inst_init(Object *obj)
>   {
> +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);
> +
> +    tpm_pt->tpm_fd = -1;
> +    tpm_pt->cancel_fd = -1;
>   }
>
>   static void tpm_passthrough_inst_finalize(Object *obj)
>   {
> +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);
> +
> +    tpm_passthrough_cancel_cmd(TPM_BACKEND(obj));
> +
> +    qemu_close(tpm_pt->tpm_fd);
> +    qemu_close(tpm_pt->cancel_fd);
>   }

With these I am wondering whether they should not have been moved here 
from tpm_passthrough_destroy()? The appear there as well.

>   static void tpm_passthrough_class_init(ObjectClass *klass, void *data)
Amarnath Valluri May 2, 2017, 7:09 a.m. UTC | #2 
On 04/25/2017 09:27 PM, Stefan Berger wrote:
> On 04/07/2017 10:30 AM, Amarnath Valluri wrote:
>> Initialize and free TPMBackend data members in it's own 
>> instance_init() and
>> instance_finalize methods.
>>
>> Took the opportunity to fix object cleanup in 
>> tpm_backend_{create,destroy}
>> methods
>>
>> Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
>> ---
>>   backends/tpm.c           |  8 ++++++--
>>   hw/tpm/tpm_passthrough.c | 18 +++++++++++-------
>>   2 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/backends/tpm.c b/backends/tpm.c
>> index 0ec0c67..99d2b2b 100644
>> --- a/backends/tpm.c
>> +++ b/backends/tpm.c
>> @@ -57,7 +57,7 @@ void tpm_backend_destroy(TPMBackend *s)
>>
>>       k->ops->destroy(s);
>>
>> -    tpm_backend_thread_end(s);
>> +    object_unref(OBJECT(s));
>>   }
>>
>>   int tpm_backend_init(TPMBackend *s, TPMState *state,
>> @@ -182,11 +182,13 @@ static void tpm_backend_prop_set_opened(Object 
>> *obj, bool value, Error **errp)
>>
>>   static void tpm_backend_instance_init(Object *obj)
>>   {
>> +    TPMBackend *s = TPM_BACKEND(obj);
>> +
>>       object_property_add_bool(obj, "opened",
>>                                tpm_backend_prop_get_opened,
>>                                tpm_backend_prop_set_opened,
>>                                NULL);
>> -
>> +    s->fe_model = -1;
>>   }
>>
>>   static void tpm_backend_instance_finalize(Object *obj)
>> @@ -194,6 +196,8 @@ static void tpm_backend_instance_finalize(Object 
>> *obj)
>>       TPMBackend *s = TPM_BACKEND(obj);
>>
>>       g_free(s->id);
>> +    g_free(s->path);
>> +    g_free(s->cancel_path);
>>       tpm_backend_thread_end(s);
>>   }
>>
>> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
>> index f50d9cf..65831b5 100644
>> --- a/hw/tpm/tpm_passthrough.c
>> +++ b/hw/tpm/tpm_passthrough.c
>> @@ -417,8 +417,6 @@ static TPMBackend 
>> *tpm_passthrough_create(QemuOpts *opts, const char *id)
>>       TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
>>
>>       tb->id = g_strdup(id);
>> -    /* let frontend set the fe_model to proper value */
>> -    tb->fe_model = -1;
>>
>>       if (tpm_passthrough_handle_device_opts(opts, tb)) {
>>           goto err_exit;
>> @@ -432,7 +430,7 @@ static TPMBackend 
>> *tpm_passthrough_create(QemuOpts *opts, const char *id)
>>       return tb;
>>
>>   err_exit:
>> -    g_free(tb->id);
>> +    object_unref(obj);
>>
>>       return NULL;
>>   }
>> @@ -445,10 +443,6 @@ static void tpm_passthrough_destroy(TPMBackend *tb)
>>
>>       qemu_close(tpm_pt->tpm_fd);
>>       qemu_close(tpm_pt->cancel_fd);
>> -
>> -    g_free(tb->id);
>> -    g_free(tb->path);
>> -    g_free(tb->cancel_path);
>
>
> Looks like here you are correcting the error from the previous patch...
>
>
>
>>       g_free(tpm_pt->tpm_dev);
>>   }
>>
>> @@ -486,10 +480,20 @@ static const TPMDriverOps 
>> tpm_passthrough_driver = {
>>
>>   static void tpm_passthrough_inst_init(Object *obj)
>>   {
>> +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);
>> +
>> +    tpm_pt->tpm_fd = -1;
>> +    tpm_pt->cancel_fd = -1;
>>   }
>>
>>   static void tpm_passthrough_inst_finalize(Object *obj)
>>   {
>> +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);
>> +
>> +    tpm_passthrough_cancel_cmd(TPM_BACKEND(obj));
>> +
>> +    qemu_close(tpm_pt->tpm_fd);
>> +    qemu_close(tpm_pt->cancel_fd);
>>   }
>
> With these I am wondering whether they should not have been moved here 
> from tpm_passthrough_destroy()? The appear there as well.
Yes, I agree with your point. Anyways,  'destroy()' method itself 
removed from interface in later commit(4c0c5e1) in the series.

- Amarnath
diff mbox

Patch

diff --git a/backends/tpm.c b/backends/tpm.c
index 0ec0c67..99d2b2b 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -57,7 +57,7 @@  void tpm_backend_destroy(TPMBackend *s)
 
     k->ops->destroy(s);
 
-    tpm_backend_thread_end(s);
+    object_unref(OBJECT(s));
 }
 
 int tpm_backend_init(TPMBackend *s, TPMState *state,
@@ -182,11 +182,13 @@  static void tpm_backend_prop_set_opened(Object *obj, bool value, Error **errp)
 
 static void tpm_backend_instance_init(Object *obj)
 {
+    TPMBackend *s = TPM_BACKEND(obj);
+
     object_property_add_bool(obj, "opened",
                              tpm_backend_prop_get_opened,
                              tpm_backend_prop_set_opened,
                              NULL);
-
+    s->fe_model = -1;
 }
 
 static void tpm_backend_instance_finalize(Object *obj)
@@ -194,6 +196,8 @@  static void tpm_backend_instance_finalize(Object *obj)
     TPMBackend *s = TPM_BACKEND(obj);
 
     g_free(s->id);
+    g_free(s->path);
+    g_free(s->cancel_path);
     tpm_backend_thread_end(s);
 }
 
diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index f50d9cf..65831b5 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -417,8 +417,6 @@  static TPMBackend *tpm_passthrough_create(QemuOpts *opts, const char *id)
     TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
 
     tb->id = g_strdup(id);
-    /* let frontend set the fe_model to proper value */
-    tb->fe_model = -1;
 
     if (tpm_passthrough_handle_device_opts(opts, tb)) {
         goto err_exit;
@@ -432,7 +430,7 @@  static TPMBackend *tpm_passthrough_create(QemuOpts *opts, const char *id)
     return tb;
 
 err_exit:
-    g_free(tb->id);
+    object_unref(obj);
 
     return NULL;
 }
@@ -445,10 +443,6 @@  static void tpm_passthrough_destroy(TPMBackend *tb)
 
     qemu_close(tpm_pt->tpm_fd);
     qemu_close(tpm_pt->cancel_fd);
-
-    g_free(tb->id);
-    g_free(tb->path);
-    g_free(tb->cancel_path);
     g_free(tpm_pt->tpm_dev);
 }
 
@@ -486,10 +480,20 @@  static const TPMDriverOps tpm_passthrough_driver = {
 
 static void tpm_passthrough_inst_init(Object *obj)
 {
+    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);
+
+    tpm_pt->tpm_fd = -1;
+    tpm_pt->cancel_fd = -1;
 }
 
 static void tpm_passthrough_inst_finalize(Object *obj)
 {
+    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);
+
+    tpm_passthrough_cancel_cmd(TPM_BACKEND(obj));
+
+    qemu_close(tpm_pt->tpm_fd);
+    qemu_close(tpm_pt->cancel_fd);
 }
 
 static void tpm_passthrough_class_init(ObjectClass *klass, void *data)