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

["Hervé Poussineau" <hpoussin@reactos.org>]

[-- Attachment #1: Type: text/plain, Size: 6689 bytes --]

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 <hpoussin@reactos.org>
>> ---
>>  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 
(assertion=assertion@entry=0x803809f8 "!enumerating",
      file=file@entry=0x80380adc "qom/object.c", line=line@entry=82,
      function=function@entry=0x80380c6c "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 (fn=fn@entry=0x80197180 
<object_class_get_list_tramp>,
      implements_type=implements_type@entry=0x8039b834 "powerpc-cpu", 
include_abstract=include_abstract@entry=false,
      opaque=opaque@entry=0xbffff078) at qom/object.c:585
  #12 0x801981ba in object_class_get_list 
(implements_type=implements_type@entry=0x8039b834 "powerpc-cpu",
      include_abstract=include_abstract@entry=false) at qom/object.c:618
  #13 0x80328d4e in ppc_cpu_class_by_name (name=name@entry=0x8039dc69 "G3")
      at target-ppc/translate_init.c:8003
  #14 0x80328f7a in cpu_ppc_init (cpu_model=cpu_model@entry=0x8039dc69 "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),
>>
> 
> 


[-- Attachment #2: 0001-usb-ehci-add-an-empty-interface-to-expose-a-problem-.patch --]
[-- Type: text/plain, Size: 3765 bytes --]

From 4da22be31d5fc8df887b8c76c609b9844bebe9f4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= <hpoussin@reactos.org>
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 (assertion=assertion@entry=0x803809f8 "!enumerating",
     file=file@entry=0x80380adc "qom/object.c", line=line@entry=82,
     function=function@entry=0x80380c6c "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 (fn=fn@entry=0x80197180 <object_class_get_list_tramp>,
     implements_type=implements_type@entry=0x8039b834 "powerpc-cpu", include_abstract=include_abstract@entry=false,
     opaque=opaque@entry=0xbffff078) at qom/object.c:585
 #12 0x801981ba in object_class_get_list (implements_type=implements_type@entry=0x8039b834 "powerpc-cpu",
     include_abstract=include_abstract@entry=false) at qom/object.c:618
 #13 0x80328d4e in ppc_cpu_class_by_name (name=name@entry=0x8039dc69 "G3")
     at target-ppc/translate_init.c:8003
 #14 0x80328f7a in cpu_ppc_init (cpu_model=cpu_model@entry=0x8039dc69 "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