qom/object.c 'if (type_table == NULL)' statement is redundant , delete it.

[PATCH] qom/object.c 'if (type_table == NULL)' statement is redundant , delete it.

Posted by zhuguanghong 4 years, 6 months ago

Signed-off-by: zhuguanghong <zhuguanghong@uniontech.com>
---
 qom/object.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 6a01d56546..c8f5481afe 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -78,9 +78,7 @@ static GHashTable *type_table_get(void)
 {
     static GHashTable *type_table;
 
-    if (type_table == NULL) {
-        type_table = g_hash_table_new(g_str_hash, g_str_equal);
-    }
+    type_table = g_hash_table_new(g_str_hash, g_str_equal);
 
     return type_table;
 }
-- 
2.20.1

Re: [PATCH] qom/object.c 'if (type_table == NULL)' statement is redundant , delete it.

Posted by Marc-André Lureau 4 years, 6 months ago

Hi

On Wed, Jul 21, 2021 at 5:22 PM zhuguanghong <zhuguanghong@uniontech.com>
wrote:

> Signed-off-by: zhuguanghong <zhuguanghong@uniontech.com>
> ---
>  qom/object.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 6a01d56546..c8f5481afe 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -78,9 +78,7 @@ static GHashTable *type_table_get(void)
>  {
>      static GHashTable *type_table;
>
> -    if (type_table == NULL) {
> -        type_table = g_hash_table_new(g_str_hash, g_str_equal);
> -    }
> +    type_table = g_hash_table_new(g_str_hash, g_str_equal);
>
>
nack.It's not redundant, it does a one-time initialization.

We may want to replace it with a more explicit and thread-safe version
though:
https://developer.gnome.org/glib/stable/glib-Threads.html#g-once-init-enter

     return type_table;
>  }
> --
> 2.20.1
>
>
>
>
>

-- 
Marc-André Lureau

Re: [PATCH] qom/object.c 'if (type_table == NULL)' statement is redundant , delete it.

Posted by Daniel P. Berrangé 4 years, 6 months ago

On Wed, Jul 21, 2021 at 05:55:44PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 21, 2021 at 5:22 PM zhuguanghong <zhuguanghong@uniontech.com>
> wrote:
> 
> > Signed-off-by: zhuguanghong <zhuguanghong@uniontech.com>
> > ---
> >  qom/object.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index 6a01d56546..c8f5481afe 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -78,9 +78,7 @@ static GHashTable *type_table_get(void)
> >  {
> >      static GHashTable *type_table;
> >
> > -    if (type_table == NULL) {
> > -        type_table = g_hash_table_new(g_str_hash, g_str_equal);
> > -    }
> > +    type_table = g_hash_table_new(g_str_hash, g_str_equal);
> >
> >
> nack.It's not redundant, it does a one-time initialization.
> 
> We may want to replace it with a more explicit and thread-safe version
> though:
> https://developer.gnome.org/glib/stable/glib-Threads.html#g-once-init-enter

I would have thought QOM usage is so widespread in QEMU that we'll
trigger initialization of this hash tble very early in startup
while we're still single threaded.

That said there's not really any harm in using g_once

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] qom/object.c 'if (type_table == NULL)' statement is redundant , delete it.

Posted by 朱光宏 4 years, 6 months ago

&nbsp;Hi
Signed-off-by: zhuguanghong <zhuguanghong@uniontech.com&gt;
---
&nbsp;qom/object.c | 4 ++++
&nbsp;1 file changed, 4 insertions(+)


diff --git a/qom/object.c b/qom/object.c
index 6a01d56546..dc0a363ed0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -73,15 +73,19 @@ struct TypeImpl
&nbsp;};


&nbsp;static Type type_interface;
+G_LOCK_DEFINE (GHashTable_mutex);


&nbsp;static GHashTable *type_table_get(void)
&nbsp;{
&nbsp; &nbsp; &nbsp;static GHashTable *type_table;
+&nbsp; &nbsp; G_LOCK ( GHashTable_mutex);&nbsp; &nbsp; &nbsp; &nbsp;&nbsp;


&nbsp; &nbsp; &nbsp;if (type_table == NULL) {
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;type_table = g_hash_table_new(g_str_hash, g_str_equal);
&nbsp; &nbsp; &nbsp;}


+&nbsp; &nbsp; G_UNLOCK ( GHashTable_mutex);&nbsp; &nbsp; &nbsp;&nbsp;
+
&nbsp; &nbsp; &nbsp;return type_table;
&nbsp;}



How about using G_LOCK ? thread safety




&nbsp;
&nbsp;
------------------&nbsp;Original&nbsp;------------------
From: &nbsp;"Daniel P. Berrangé"<berrange@redhat.com&gt;;
Date: &nbsp;Wed, Jul 21, 2021 10:00 PM
To: &nbsp;"Marc-André Lureau"<marcandre.lureau@gmail.com&gt;; 
Cc: &nbsp;"zhuguanghong"<zhuguanghong@uniontech.com&gt;; "Paolo Bonzini"<pbonzini@redhat.com&gt;; "Eduardo Habkost"<ehabkost@redhat.com&gt;; "QEMU"<qemu-devel@nongnu.org&gt;; 
Subject: &nbsp;Re: [PATCH] qom/object.c 'if (type_table == NULL)' statement is redundant , delete it.

&nbsp;

On Wed, Jul 21, 2021 at 05:55:44PM +0400, Marc-André Lureau wrote:
&gt; Hi
&gt; 
&gt; On Wed, Jul 21, 2021 at 5:22 PM zhuguanghong <zhuguanghong@uniontech.com&gt;
&gt; wrote:
&gt; 
&gt; &gt; Signed-off-by: zhuguanghong <zhuguanghong@uniontech.com&gt;
&gt; &gt; ---
&gt; &gt;&nbsp; qom/object.c | 4 +---
&gt; &gt;&nbsp; 1 file changed, 1 insertion(+), 3 deletions(-)
&gt; &gt;
&gt; &gt; diff --git a/qom/object.c b/qom/object.c
&gt; &gt; index 6a01d56546..c8f5481afe 100644
&gt; &gt; --- a/qom/object.c
&gt; &gt; +++ b/qom/object.c
&gt; &gt; @@ -78,9 +78,7 @@ static GHashTable *type_table_get(void)
&gt; &gt;&nbsp; {
&gt; &gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; static GHashTable *type_table;
&gt; &gt;
&gt; &gt; -&nbsp;&nbsp;&nbsp; if (type_table == NULL) {
&gt; &gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; type_table = g_hash_table_new(g_str_hash, g_str_equal);
&gt; &gt; -&nbsp;&nbsp;&nbsp; }
&gt; &gt; +&nbsp;&nbsp;&nbsp; type_table = g_hash_table_new(g_str_hash, g_str_equal);
&gt; &gt;
&gt; &gt;
&gt; nack.It's not redundant, it does a one-time initialization.
&gt; 
&gt; We may want to replace it with a more explicit and thread-safe version
&gt; though:
&gt; https://developer.gnome.org/glib/stable/glib-Threads.html#g-once-init-enter

I would have thought QOM usage is so widespread in QEMU that we'll
trigger initialization of this hash tble very early in startup
while we're still single threaded.

That said there's not really any harm in using g_once

Regards,
Daniel
-- 
|: https://berrange.com&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; -o-&nbsp;&nbsp;&nbsp; https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; -o-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; https://fstop138.berrange.com :|
|: https://entangle-photo.org&nbsp;&nbsp;&nbsp; -o-&nbsp;&nbsp;&nbsp; https://www.instagram.com/dberrange :|