Re: [Qemu-devel] [PATCH 2/7] qom: handle registration of new types when initializing the first ones

[Hervé Poussineau]

Andreas Färber a écrit :
> Am 02.05.2013 22:08, schrieb Hervé Poussineau:
> > When initializing all types in object_class_foreach, called by 
> > object_class_get_list,
> > some new types may be registered. Those will change the type internal hashtable 
> > which
> > is currently enumerated, and may crash QEMU.
> >
> > Fix it, by adding a second hash table which contains all the non-initialized 
> > types,
> > merged to the main one before each round of initializations.
> >
> > Bug has been detected when registering dynamic types containing an interface.
> >
> > Signed-off-by: Hervé Poussineau <address@hidden>
> > ---
> >  qom/object.c |   45 +++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 37 insertions(+), 8 deletions(-)
>
> Could you be more specific about how to reproduce the problem? Is it a
> generic issue or specific to some later patch in this series? I find
> neither object_class_get_list() nor object_class_for_each() being used
> in this series. And registering types during object_class_for_each()
> doesn't sound right... CC'ing Anthony and Paolo.

Try the attached patch, and run with qemu-system-ppc (no arguments)
I added a dummy interface to a random device, but the problem should be 
exposed by whatever interface on whatever device. I saw the problem in 
patch 5/7 ("add a Nvram interface").
However, the problem doesn't seem to appear on other system emulations 
like i386.

With attached patch, you'll get an assert:
qemu-system-ppc: qom/object.c:82: type_table_add: Assertion 
`!enumerating' failed.

Program received signal SIGABRT, Aborted.
0xb7fe1430 in __kernel_vsyscall ()
(gdb) bt
 #0  0xb7fe1430 in __kernel_vsyscall ()
 #1  0xb6f27941 in *__GI_raise (sig=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:64
 #2  0xb6f2ad72 in *__GI_abort () at abort.c:92
 #3  0xb6f20b58 in *__GI___assert_fail 
(address@hidden "!enumerating",
     address@hidden "qom/object.c", address@hidden,
     address@hidden "type_table_add") at assert.c:81
 #4  0x80197513 in type_table_add (ti=0x80b67bd0) at qom/object.c:82
 #5  type_register_internal (info=0xbfffef0c) at qom/object.c:124
 #6  0x8019764c in type_initialize_interface (parent=0x80b3ec18 
"interface",
     ti=<error reading variable: Unhandled dwarf expression opcode 0xfa>,
     ti=<error reading variable: Unhandled dwarf expression opcode 
0xfa>) at qom/object.c:218
 #7  0x801978fe in type_initialize (ti=<optimized out>) at qom/object.c:271
 #8  type_initialize (ti=0x80b3eb30) at qom/object.c:229
 #9  0x80197dfa in object_class_foreach_tramp (key=0x80b3ebf0, 
value=0x80b3eb30, opaque=0xbffff03c)
     at qom/object.c:563
 #10 0xb7ef35e2 in g_hash_table_foreach () from 
/lib/i386-linux-gnu/libglib-2.0.so.0
 #11 0x801980b1 in object_class_foreach (address@hidden 
<object_class_get_list_tramp>,
     address@hidden "powerpc-cpu", 
address@hidden,
     address@hidden) at qom/object.c:585
 #12 0x801981ba in object_class_get_list 
(address@hidden "powerpc-cpu",
     address@hidden) at qom/object.c:618
 #13 0x80328d4e in ppc_cpu_class_by_name (address@hidden "G3")
     at target-ppc/translate_init.c:8003
 #14 0x80328f7a in cpu_ppc_init (address@hidden "G3")
     at target-ppc/translate_init.c:8020
 #15 0x80216724 in ppc_heathrow_init (args=0xbffff2a8) at 
hw/ppc/mac_oldworld.c:109
 #16 0x80040b81 in main (argc=1, argv=0xbffff4b4, envp=0xbffff4bc) at 
vl.c:4304
> > diff --git a/qom/object.c b/qom/object.c
> > index 75e6aac..e0a24dc 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -65,25 +65,39 @@ struct TypeImpl
> >  
> >  static Type type_interface;
> >  
> > +static GHashTable *type_table_to_initialize;
> > +static GHashTable *type_table_initialized;
> > +
> >  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);
> > +    if (!type_table_initialized) {
> > +        type_table_initialized = g_hash_table_new(g_str_hash, g_str_equal);
> >      }
> >  
> > -    return type_table;
> > +    return type_table_initialized;
> >  }
> >  
> >  static void type_table_add(TypeImpl *ti)
> >  {
> > -    g_hash_table_insert(type_table_get(), (void *)ti->name, ti);
> > +    GHashTable **type_table;
> > +    if (ti->class) {
> > +        type_table = &type_table_initialized;
> > +    } else {
> > +        type_table = &type_table_to_initialize;
> > +    }
> > +    if (!*type_table) {
> > +        *type_table = g_hash_table_new(g_str_hash, g_str_equal);
> > +    }
> > +    g_hash_table_insert(*type_table, (void *)ti->name, ti);
> >  }
> >  
> >  static TypeImpl *type_table_lookup(const char *name)
> >  {
> > -    return g_hash_table_lookup(type_table_get(), name);
> > +    TypeImpl *ret = g_hash_table_lookup(type_table_get(), name);
> > +    if (!ret && type_table_to_initialize) {
> > +        ret = g_hash_table_lookup(type_table_to_initialize, name);
> > +    }
> > +    return ret;
> >  }
> >  
> >  static TypeImpl *type_register_internal(const TypeInfo *info)
> > @@ -573,13 +587,28 @@ static void object_class_foreach_tramp(gpointer key, 
> > gpointer value,
> >      data->fn(k, data->opaque);
> >  }
> >  
> > +static void object_class_merge(gpointer key, gpointer value,
> > +                               gpointer opaque)
> > +{
> > +    g_hash_table_insert(type_table_get(), key, value);
> > +}
> > +
> >  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
> >                            const char *implements_type, bool include_abstract,
> >                            void *opaque)
> >  {
> >      OCFData data = { fn, implements_type, include_abstract, opaque };
> >  
> > -    g_hash_table_foreach(type_table_get(), object_class_foreach_tramp, &data);
> > +    while (type_table_to_initialize &&
> > +           g_hash_table_size(type_table_to_initialize) > 0) {
> > +        g_hash_table_foreach(type_table_to_initialize, object_class_merge,
> > +                             NULL);
> > +        g_hash_table_destroy(type_table_to_initialize);
> > +        type_table_to_initialize = NULL;
> > +
> > +        g_hash_table_foreach(type_table_get(), object_class_foreach_tramp,
> > +                             &data);
> > +    }
> >  }
> >  
> >  int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),

From 4da22be31d5fc8df887b8c76c609b9844bebe9f4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= <address@hidden>
Date: Sun, 5 May 2013 10:31:24 +0200
Subject: [PATCH] usb-ehci: add an empty interface to expose a problem in QOM

Run with qemu-system-ppc (no arguments)

qemu-system-ppc: qom/object.c:82: type_table_add: Assertion `!enumerating' 
failed.

Program received signal SIGABRT, Aborted.
0xb7fe1430 in __kernel_vsyscall ()
(gdb) bt
 #0  0xb7fe1430 in __kernel_vsyscall ()
 #1  0xb6f27941 in *__GI_raise (sig=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:64
 #2  0xb6f2ad72 in *__GI_abort () at abort.c:92
 #3  0xb6f20b58 in *__GI___assert_fail (address@hidden "!enumerating",
     address@hidden "qom/object.c", address@hidden,
     address@hidden "type_table_add") at assert.c:81
 #4  0x80197513 in type_table_add (ti=0x80b67bd0) at qom/object.c:82
 #5  type_register_internal (info=0xbfffef0c) at qom/object.c:124
 #6  0x8019764c in type_initialize_interface (parent=0x80b3ec18 "interface",
     ti=<error reading variable: Unhandled dwarf expression opcode 0xfa>,
     ti=<error reading variable: Unhandled dwarf expression opcode 0xfa>) at 
qom/object.c:218
 #7  0x801978fe in type_initialize (ti=<optimized out>) at qom/object.c:271
 #8  type_initialize (ti=0x80b3eb30) at qom/object.c:229
 #9  0x80197dfa in object_class_foreach_tramp (key=0x80b3ebf0, 
value=0x80b3eb30, opaque=0xbffff03c)
     at qom/object.c:563
 #10 0xb7ef35e2 in g_hash_table_foreach () from 
/lib/i386-linux-gnu/libglib-2.0.so.0
 #11 0x801980b1 in object_class_foreach (address@hidden 
<object_class_get_list_tramp>,
     address@hidden "powerpc-cpu", address@hidden,
     address@hidden) at qom/object.c:585
 #12 0x801981ba in object_class_get_list (address@hidden "powerpc-cpu",
     address@hidden) at qom/object.c:618
 #13 0x80328d4e in ppc_cpu_class_by_name (address@hidden "G3")
     at target-ppc/translate_init.c:8003
 #14 0x80328f7a in cpu_ppc_init (address@hidden "G3")
     at target-ppc/translate_init.c:8020
 #15 0x80216724 in ppc_heathrow_init (args=0xbffff2a8) at 
hw/ppc/mac_oldworld.c:109
 #16 0x80040b81 in main (argc=1, argv=0xbffff4b4, envp=0xbffff4bc) at vl.c:4304
---
 hw/usb/hcd-uhci.c |    4 ++++
 qom/object.c      |    4 ++++
 2 files changed, 8 insertions(+)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index f8c4286..a95ca30 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1386,6 +1386,10 @@ static void uhci_register_types(void)
         .instance_size = sizeof(UHCIState),
         .class_size    = sizeof(UHCIPCIDeviceClass),
         .class_init    = uhci_class_init,
+        .interfaces = (InterfaceInfo[]) {
+            { TYPE_INTERFACE },
+            { }
+        }
     };
     int i;
 
diff --git a/qom/object.c b/qom/object.c
index 75e6aac..b8e9f4f 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -76,8 +76,10 @@ static GHashTable *type_table_get(void)
     return type_table;
 }
 
+static bool enumerating = false;
 static void type_table_add(TypeImpl *ti)
 {
+    assert(!enumerating);
     g_hash_table_insert(type_table_get(), (void *)ti->name, ti);
 }
 
@@ -579,7 +581,9 @@ void object_class_foreach(void (*fn)(ObjectClass *klass, 
void *opaque),
 {
     OCFData data = { fn, implements_type, include_abstract, opaque };
 
+    enumerating = true;
     g_hash_table_foreach(type_table_get(), object_class_foreach_tramp, &data);
+    enumerating = false;
 }
 
 int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
-- 
1.7.10.4