[Qemu-devel] [PATCH 0/2] qom: fix registration of QOM interfaces

[Qemu-devel] [PATCH 0/2] qom: fix registration of QOM interfaces

[Paolo Bonzini]

object_class_foreach can initialize new types, and also register new types
corresponding to the implementations of interfaces. Those will change the
type internal hashtable which is currently enumerated, and GHashTable does
not permit this.

Fix this by not registering those internal types in the hash table, and
assert that we do not reenter GHashTable incorrectly during iteration.

Hervé Poussineau (1):
  qom: detect bad reentrance during object_class_foreach

Paolo Bonzini (1):
  qom: do not register interface "types" in the type table

 qom/object.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH 1/2] qom: do not register interface "types" in the type table

[Paolo Bonzini]

There should be no need to look them up nor enumerate the interface
"types", whose "classes" are really just vtables.  Just create the
types and add them to the interface list of the parent type.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index fc19cf6..3a43186 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -88,7 +88,7 @@ static TypeImpl *type_table_lookup(const char *name)
     return g_hash_table_lookup(type_table_get(), name);
 }
 
-static TypeImpl *type_register_internal(const TypeInfo *info)
+static TypeImpl *type_new(const TypeInfo *info)
 {
     TypeImpl *ti = g_malloc0(sizeof(*ti));
     int i;
@@ -122,8 +122,15 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
     }
     ti->num_interfaces = i;
 
-    type_table_add(ti);
+    return ti;
+}
 
+static TypeImpl *type_register_internal(const TypeInfo *info)
+{
+    TypeImpl *ti;
+    ti = type_new(info);
+
+    type_table_add(ti);
     return ti;
 }
 
@@ -216,7 +223,7 @@ static void type_initialize_interface(TypeImpl *ti, const char *parent)
     info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
     info.abstract = true;
 
-    iface_impl = type_register(&info);
+    iface_impl = type_new(&info);
     type_initialize(iface_impl);
     g_free((char *)info.name);
 
-- 
1.8.4.2

[Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Paolo Bonzini]

From: Hervé Poussineau <hpoussin@reactos.org>

We should not modify the type hash table while it is being iterated on.
Assert that it does not happen.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 3a43186..1dee9f0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -78,8 +78,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);
 }
 
@@ -666,7 +668,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.8.4.2

Re: [Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 1:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Hervé Poussineau <hpoussin@reactos.org>
>
> We should not modify the type hash table while it is being iterated on.
> Assert that it does not happen.
>
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/qom/object.c b/qom/object.c
> index 3a43186..1dee9f0 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -78,8 +78,10 @@ static GHashTable *type_table_get(void)
>      return type_table;
>  }
>
> +static bool enumerating = false;

Global variable could probably use a more descriptive name.

Regards,
Peter

>  static void type_table_add(TypeImpl *ti)
>  {
> +    assert(!enumerating);
>      g_hash_table_insert(type_table_get(), (void *)ti->name, ti);
>  }
>
> @@ -666,7 +668,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.8.4.2
>
>

Re: [Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Andreas Färber]

Am 04.12.2013 06:51, schrieb Peter Crosthwaite:
> On Wed, Dec 4, 2013 at 1:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> From: Hervé Poussineau <hpoussin@reactos.org>
>>
>> We should not modify the type hash table while it is being iterated on.
>> Assert that it does not happen.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  qom/object.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 3a43186..1dee9f0 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -78,8 +78,10 @@ static GHashTable *type_table_get(void)
>>      return type_table;
>>  }
>>
>> +static bool enumerating = false;
> 
> Global variable could probably use a more descriptive name.

I renamed it to enumerating_types and dropped the assignment as
suggested elsewhere by Alexey (a reply here would've been nice!).

I also took the liberty of inserted a white line to make the function
better readable.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/2] qom: fix registration of QOM interfaces

[Andreas Färber]

Am 03.12.2013 16:41, schrieb Paolo Bonzini:
> Hervé Poussineau (1):
>   qom: detect bad reentrance during object_class_foreach
> 
> Paolo Bonzini (1):
>   qom: do not register interface "types" in the type table

Thanks, applied to qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Alexey Kardashevskiy]

On 12/16/2013 08:23 AM, Andreas Färber wrote:
> Am 04.12.2013 06:51, schrieb Peter Crosthwaite:
>> On Wed, Dec 4, 2013 at 1:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>
>>> We should not modify the type hash table while it is being iterated on.
>>> Assert that it does not happen.
>>>
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  qom/object.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 3a43186..1dee9f0 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -78,8 +78,10 @@ static GHashTable *type_table_get(void)
>>>      return type_table;
>>>  }
>>>
>>> +static bool enumerating = false;
>>
>> Global variable could probably use a more descriptive name.
> 
> I renamed it to enumerating_types and dropped the assignment as
> suggested elsewhere by Alexey (a reply here would've been nice!).

Whose reply? To what? :)

> 
> I also took the liberty of inserted a white line to make the function
> better readable.

I do not mind, this was not my stuff :)

What I wonder about is what is going to happen to the rest of what I
posted? Should I wait till this qom-next gets merged to upstream and repost
my patches for Alex Graf again? Thanks.



-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Andreas Färber]

Am 19.12.2013 12:38, schrieb Alexey Kardashevskiy:
> On 12/16/2013 08:23 AM, Andreas Färber wrote:
>> Am 04.12.2013 06:51, schrieb Peter Crosthwaite:
>>> On Wed, Dec 4, 2013 at 1:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>
>>>> We should not modify the type hash table while it is being iterated on.
>>>> Assert that it does not happen.
>>>>
>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>  qom/object.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/qom/object.c b/qom/object.c
>>>> index 3a43186..1dee9f0 100644
>>>> --- a/qom/object.c
>>>> +++ b/qom/object.c
>>>> @@ -78,8 +78,10 @@ static GHashTable *type_table_get(void)
>>>>      return type_table;
>>>>  }
>>>>
>>>> +static bool enumerating = false;
>>>
>>> Global variable could probably use a more descriptive name.
>>
>> I renamed it to enumerating_types and dropped the assignment as
>> suggested elsewhere by Alexey (a reply here would've been nice!).
> 
> Whose reply? To what? :)

A reply of yours to Peter C.'s change request, stating that you have
addressed it - and in this case in which series. :)

>> I also took the liberty of inserted a white line to make the function
>> better readable.
> 
> I do not mind, this was not my stuff :)
> 
> What I wonder about is what is going to happen to the rest of what I
> posted? Should I wait till this qom-next gets merged to upstream and repost
> my patches for Alex Graf again? Thanks.

I do intend to post a pull before Chistmas, reviewing+applying the last
no_user patch from Markus was one key ingredient for that.

Not sure which rest exactly you're referring to? I need to review your
latest QemuOpts proposal among others. Actually I have a whole A4 page
written down with series I'm wading through. ;)

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Alexey Kardashevskiy]

On 12/20/2013 10:29 PM, Andreas Färber wrote:
> Am 19.12.2013 12:38, schrieb Alexey Kardashevskiy:
>> On 12/16/2013 08:23 AM, Andreas Färber wrote:
>>> Am 04.12.2013 06:51, schrieb Peter Crosthwaite:
>>>> On Wed, Dec 4, 2013 at 1:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>>
>>>>> We should not modify the type hash table while it is being iterated on.
>>>>> Assert that it does not happen.
>>>>>
>>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> ---
>>>>>  qom/object.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/qom/object.c b/qom/object.c
>>>>> index 3a43186..1dee9f0 100644
>>>>> --- a/qom/object.c
>>>>> +++ b/qom/object.c
>>>>> @@ -78,8 +78,10 @@ static GHashTable *type_table_get(void)
>>>>>      return type_table;
>>>>>  }
>>>>>
>>>>> +static bool enumerating = false;
>>>>
>>>> Global variable could probably use a more descriptive name.
>>>
>>> I renamed it to enumerating_types and dropped the assignment as
>>> suggested elsewhere by Alexey (a reply here would've been nice!).
>>
>> Whose reply? To what? :)
> 
> A reply of yours to Peter C.'s change request, stating that you have
> addressed it - and in this case in which series. :)

You lost me here. I got these patches from Paolo Bonzini and I was told to
repost them together with my other patches, that's it :)


>>> I also took the liberty of inserted a white line to make the function
>>> better readable.
>>
>> I do not mind, this was not my stuff :)
>>
>> What I wonder about is what is going to happen to the rest of what I
>> posted? Should I wait till this qom-next gets merged to upstream and repost
>> my patches for Alex Graf again? Thanks.
> 
> I do intend to post a pull before Chistmas, reviewing+applying the last
> no_user patch from Markus was one key ingredient for that.
> 
> Not sure which rest exactly you're referring to? I need to review your
> latest QemuOpts proposal among others. Actually I have a whole A4 page
> written down with series I'm wading through. ;)


I posted the "[PATCH v4 0/8] spapr: bootindex support" series which
included the 2 patches you mentioned in this series, from the rest - 3 are
for spapr and 3 are platform independent.



-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Alexey Kardashevskiy]

On 12/20/2013 10:43 PM, Alexey Kardashevskiy wrote:
> On 12/20/2013 10:29 PM, Andreas Färber wrote:
>> Am 19.12.2013 12:38, schrieb Alexey Kardashevskiy:
>>> On 12/16/2013 08:23 AM, Andreas Färber wrote:
>>>> Am 04.12.2013 06:51, schrieb Peter Crosthwaite:
>>>>> On Wed, Dec 4, 2013 at 1:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>>>
>>>>>> We should not modify the type hash table while it is being iterated on.
>>>>>> Assert that it does not happen.
>>>>>>
>>>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> ---
>>>>>>  qom/object.c | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/qom/object.c b/qom/object.c
>>>>>> index 3a43186..1dee9f0 100644
>>>>>> --- a/qom/object.c
>>>>>> +++ b/qom/object.c
>>>>>> @@ -78,8 +78,10 @@ static GHashTable *type_table_get(void)
>>>>>>      return type_table;
>>>>>>  }
>>>>>>
>>>>>> +static bool enumerating = false;
>>>>>
>>>>> Global variable could probably use a more descriptive name.
>>>>
>>>> I renamed it to enumerating_types and dropped the assignment as
>>>> suggested elsewhere by Alexey (a reply here would've been nice!).
>>>
>>> Whose reply? To what? :)
>>
>> A reply of yours to Peter C.'s change request, stating that you have
>> addressed it - and in this case in which series. :)
> 
> You lost me here. I got these patches from Paolo Bonzini and I was told to
> repost them together with my other patches, that's it :)
> 
> 
>>>> I also took the liberty of inserted a white line to make the function
>>>> better readable.
>>>
>>> I do not mind, this was not my stuff :)
>>>
>>> What I wonder about is what is going to happen to the rest of what I
>>> posted? Should I wait till this qom-next gets merged to upstream and repost
>>> my patches for Alex Graf again? Thanks.
>>
>> I do intend to post a pull before Chistmas, reviewing+applying the last
>> no_user patch from Markus was one key ingredient for that.
>>
>> Not sure which rest exactly you're referring to? I need to review your
>> latest QemuOpts proposal among others. Actually I have a whole A4 page
>> written down with series I'm wading through. ;)
> 
> 
> I posted the "[PATCH v4 0/8] spapr: bootindex support" series which
> included the 2 patches you mentioned in this series, from the rest - 3 are
> for spapr and 3 are platform independent.



So? What do I do with my 6 patches from the "[PATCH v4 0/8] spapr:
bootindex support" series? Thanks!


-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] qom: detect bad reentrance during object_class_foreach

[Andreas Färber]

Am 24.12.2013 07:49, schrieb Alexey Kardashevskiy:
> On 12/20/2013 10:43 PM, Alexey Kardashevskiy wrote:
>> On 12/20/2013 10:29 PM, Andreas Färber wrote:
>>> Am 19.12.2013 12:38, schrieb Alexey Kardashevskiy:
>>>> What I wonder about is what is going to happen to the rest of what I
>>>> posted? Should I wait till this qom-next gets merged to upstream and repost
>>>> my patches for Alex Graf again? Thanks.
>>>
>>> I do intend to post a pull before Chistmas, reviewing+applying the last
>>> no_user patch from Markus was one key ingredient for that.
>>>
>>> Not sure which rest exactly you're referring to? I need to review your
>>> latest QemuOpts proposal among others. Actually I have a whole A4 page
>>> written down with series I'm wading through. ;)
>>
>>
>> I posted the "[PATCH v4 0/8] spapr: bootindex support" series which
>> included the 2 patches you mentioned in this series, from the rest - 3 are
>> for spapr and 3 are platform independent.
> 
> 
> 
> So? What do I do with my 6 patches from the "[PATCH v4 0/8] spapr:
> bootindex support" series? Thanks!

Be patient. ;)

1) I am on holidays and thus not available full-time.
2) Alex has already sent out his ppc PULL, so no urgency.
3) As mentioned, I plan to post QOM and CPU PULLs later today.

And no, you don't need to repost after parts of your series get applied
pretty much unmodified.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg