From: Marc-André Lureau <marcandre.lureau@redhat.com>

Since commit 9859fac ("object: release all props"), the code was changed
to tracks the already released properties in a hash table. I am not sure
why this was done, perhaps to prevent from potential crashes if
properties are being added dynamically during release. I am not sure if
it's a valid concern though.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/object.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 664f0f24ae..a2ccebca6d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -656,21 +656,13 @@ static void object_property_del_all(Object *obj)
     g_autoptr(GHashTable) done = g_hash_table_new(NULL, NULL);
     ObjectProperty *prop;
     ObjectPropertyIterator iter;
-    bool released;
 
-    do {
-        released = false;
-        object_property_iter_init(&iter, obj);
-        while ((prop = object_property_iter_next(&iter)) != NULL) {
-            if (g_hash_table_add(done, prop)) {
-                if (prop->release) {
-                    prop->release(obj, prop->name, prop->opaque);
-                    released = true;
-                    break;
-                }
-            }
+    object_property_iter_init(&iter, obj);
+    while ((prop = object_property_iter_next(&iter)) != NULL) {
+        if (prop->release) {
+            prop->release(obj, prop->name, prop->opaque);
         }
-    } while (released);
+    }
 
     g_hash_table_unref(obj->properties);
 }
-- 
2.49.0

Il mar 29 apr 2025, 16:03 <marcandre.lureau@redhat.com> ha scritto:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Since commit 9859fac ("object: release all props"), the code was changed
> to tracks the already released properties in a hash table. I am not sure
> why this was done, perhaps to prevent from potential crashes if
> properties are being added dynamically during release. I am not sure if
> it's a valid concern though.
>

You always need object_property_iter_init in case prop->release deletes a
property, thus invalidating the GHashTable iterator. The hash table instead
is needed in case prop->release does *not* delete a property, because then
the property reappears on subsequent recreations of the iterator.

Paolo


> -    bool released;
>
> -    do {
> -        released = false;
> -        object_property_iter_init(&iter, obj);
> -        while ((prop = object_property_iter_next(&iter)) != NULL) {
> -            if (g_hash_table_add(done, prop)) {
> -                if (prop->release) {
> -                    prop->release(obj, prop->name, prop->opaque);
> -                    released = true;
> -                    break;
> -                }
> -            }
> +    object_property_iter_init(&iter, obj);
> +    while ((prop = object_property_iter_next(&iter)) != NULL) {
> +        if (prop->release) {
> +            prop->release(obj, prop->name, prop->opaque);
>          }
> -    } while (released);
> +    }
>
>      g_hash_table_unref(obj->properties);
>  }
> --
> 2.49.0
>
>

Hi

On Tue, Apr 29, 2025 at 6:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> Il mar 29 apr 2025, 16:03 <marcandre.lureau@redhat.com> ha scritto:
>>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Since commit 9859fac ("object: release all props"), the code was changed
>> to tracks the already released properties in a hash table. I am not sure
>> why this was done, perhaps to prevent from potential crashes if
>> properties are being added dynamically during release. I am not sure if
>> it's a valid concern though.
>
>
> You always need object_property_iter_init in case prop->release deletes a property, thus invalidating the GHashTable iterator. The hash table instead is needed in case prop->release does *not* delete a property, because then the property reappears on subsequent recreations of the iterator.

Yes, changing the properties while they are being removed is I think
the reason I added the HashTable/Set. But is this a valid concern?
(the penalty is a bit sad)

Il mar 29 apr 2025, 16:37 Marc-André Lureau <marcandre.lureau@redhat.com>
ha scritto:

> Hi
>
> On Tue, Apr 29, 2025 at 6:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >
> >
> > Il mar 29 apr 2025, 16:03 <marcandre.lureau@redhat.com> ha scritto:
> >>
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> Since commit 9859fac ("object: release all props"), the code was changed
> >> to tracks the already released properties in a hash table. I am not sure
> >> why this was done, perhaps to prevent from potential crashes if
> >> properties are being added dynamically during release. I am not sure if
> >> it's a valid concern though.
> >
> >
> > You always need object_property_iter_init in case prop->release deletes
> a property, thus invalidating the GHashTable iterator. The hash table
> instead is needed in case prop->release does *not* delete a property,
> because then the property reappears on subsequent recreations of the
> iterator.
>
> Yes, changing the properties while they are being removed is I think
> the reason I added the HashTable/Set. But is this a valid concern?
> (the penalty is a bit sad)
>

I think deleting is a possibility... For example releasing a property could
trigger an object_unparent(). Since there is no better way to notice I
think it's better to keep it.

Maybe we could add a generation count to the Object struct?

Paolo

>