Implement a visitor that frees the pointer members of the visited QAPI
object in the same way that qapi_dealloc_visitor does, but similarly
to qobject_input_visitor, takes an input QObject that will dictate
which members get freed and which don't. Members not present in the
input QObject will be left unchanged in the visited QAPI object.

This is useful to free memory just before perfoming a visit with
qobject_input_visitor on a pre-existing, non-null QAPI object. If the
same QObject is passed to both visitors, the pointers overwritten by
the input visitor match the ones that are freed by the dealloc
visitor.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/qapi/dealloc-visitor.h |   6 ++
 qapi/qapi-dealloc-visitor.c    | 173 ++++++++++++++++++++++++++++++++-
 2 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
index c36715fdf3..96c7bf35c3 100644
--- a/include/qapi/dealloc-visitor.h
+++ b/include/qapi/dealloc-visitor.h
@@ -25,4 +25,10 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
  */
 Visitor *qapi_dealloc_visitor_new(void);
 
+/*
+ * Like qapi_dealloc_visitor_new but visits a QObject and only frees
+ * present members.
+ */
+Visitor *qapi_dealloc_present_visitor_new(QObject *);
+
 #endif
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 57a2c904bb..90b017cc93 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -14,14 +14,146 @@
 
 #include "qemu/osdep.h"
 #include "qapi/dealloc-visitor.h"
+#include "qemu/queue.h"
+#include "qobject/qdict.h"
+#include "qobject/qlist.h"
 #include "qobject/qnull.h"
 #include "qapi/visitor-impl.h"
 
+typedef struct QStackEntry {
+    QObject *obj;               /* QDict or QList being visited */
+    void *qapi;
+    const QListEntry *entry;    /* If @obj is QList: unvisited tail */
+    QSLIST_ENTRY(QStackEntry) node;
+} QStackEntry;
+
 struct QapiDeallocVisitor
 {
     Visitor visitor;
+    QObject *root;
+    QSLIST_HEAD(, QStackEntry) stack;
 };
 
+static void qapi_dealloc_pop(Visitor *v, void **obj)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+    assert(se && se->qapi == obj);
+    QSLIST_REMOVE_HEAD(&qdv->stack, node);
+    g_free(se);
+}
+
+static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = g_new0(QStackEntry, 1);
+
+    assert(obj);
+    se->obj = obj;
+    se->qapi = qapi;
+
+    if (qobject_type(obj) == QTYPE_QLIST) {
+        se->entry = qlist_first(qobject_to(QList, obj));
+    }
+
+    QSLIST_INSERT_HEAD(&qdv->stack, se, node);
+}
+
+static QObject *qapi_dealloc_try_get_object(QapiDeallocVisitor *qdv, const char *name)
+{
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+    QObject *qobj;
+    QObject *ret = NULL;
+
+    if (!se) {
+        assert(qdv->root);
+        return qdv->root;
+    }
+
+    qobj = se->obj;
+    assert(qobj);
+
+    if (qobject_type(qobj) == QTYPE_QDICT) {
+        assert(name);
+        ret = qdict_get(qobject_to(QDict, qobj), name);
+    } else {
+        assert(qobject_type(qobj) == QTYPE_QLIST);
+        assert(!name);
+        if (se->entry) {
+            ret = qlist_entry_obj(se->entry);
+        }
+    }
+
+    return ret;
+}
+
+static bool qapi_dealloc_present_start_struct(Visitor *v, const char *name,
+                                              void **obj, size_t size,
+                                              Error **errp)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+    if (!qobj) {
+        return false;
+    }
+    assert(qobject_type(qobj) == QTYPE_QDICT);
+    qapi_dealloc_push(v, qobj, obj);
+    return true;
+}
+
+static void qapi_dealloc_present_end_struct(Visitor *v, void **obj)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+    assert(qobject_type(se->obj) == QTYPE_QDICT);
+    qapi_dealloc_pop(v, obj);
+
+    if (obj) {
+        g_free(*obj);
+    }
+}
+
+static bool qapi_dealloc_present_start_list(Visitor *v, const char *name,
+                                            GenericList **list, size_t size,
+                                            Error **errp)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+    if (!qobj) {
+        return false;
+    }
+    assert(qobject_type(qobj) == QTYPE_QLIST);
+    qapi_dealloc_push(v, qobj, list);
+    return true;
+}
+
+static void qapi_dealloc_present_end_list(Visitor *v, void **obj)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+    assert(qobject_type(se->obj) == QTYPE_QLIST);
+    qapi_dealloc_pop(v, obj);
+}
+
+static void qapi_dealloc_present_free(Visitor *v)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+
+    while (!QSLIST_EMPTY(&qdv->stack)) {
+        QStackEntry *se = QSLIST_FIRST(&qdv->stack);
+
+        QSLIST_REMOVE_HEAD(&qdv->stack, node);
+        g_free(se);
+    }
+    qobject_unref(qdv->root);
+    g_free(qdv);
+}
+
 static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
                                       size_t unused, Error **errp)
 {
@@ -35,6 +167,21 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
     }
 }
 
+static bool qapi_dealloc_start_alternate(Visitor *v, const char *name,
+                                         GenericAlternate **obj, size_t size,
+                                         Error **errp)
+{
+    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
+    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
+
+    if (!qobj) {
+        return false;
+    }
+    assert(*obj);
+    (*obj)->type = qobject_type(qobj);
+    return true;
+}
+
 static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
 {
     if (obj) {
@@ -117,13 +264,14 @@ static void qapi_dealloc_free(Visitor *v)
     g_free(container_of(v, QapiDeallocVisitor, visitor));
 }
 
-Visitor *qapi_dealloc_visitor_new(void)
+static QapiDeallocVisitor *qapi_dealloc_visitor_new_base(void)
 {
     QapiDeallocVisitor *v;
 
     v = g_malloc0(sizeof(*v));
 
     v->visitor.type = VISITOR_DEALLOC;
+
     v->visitor.start_struct = qapi_dealloc_start_struct;
     v->visitor.end_struct = qapi_dealloc_end_struct;
     v->visitor.end_alternate = qapi_dealloc_end_alternate;
@@ -139,5 +287,28 @@ Visitor *qapi_dealloc_visitor_new(void)
     v->visitor.type_null = qapi_dealloc_type_null;
     v->visitor.free = qapi_dealloc_free;
 
+    return v;
+}
+
+Visitor *qapi_dealloc_visitor_new(void)
+{
+    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
+
+    return &v->visitor;
+}
+
+Visitor *qapi_dealloc_present_visitor_new(QObject *obj)
+{
+    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
+
+    v->visitor.start_alternate = qapi_dealloc_start_alternate;
+    v->visitor.start_list = qapi_dealloc_present_start_list;
+    v->visitor.end_list = qapi_dealloc_present_end_list;
+    v->visitor.start_struct = qapi_dealloc_present_start_struct;
+    v->visitor.end_struct = qapi_dealloc_present_end_struct;
+    v->visitor.free = qapi_dealloc_present_free;
+
+    v->root = qobject_ref(obj);
+
     return &v->visitor;
 }
-- 
2.51.0

On Mon, Feb 02, 2026 at 07:40:56PM -0300, Fabiano Rosas wrote:
> Implement a visitor that frees the pointer members of the visited QAPI
> object in the same way that qapi_dealloc_visitor does, but similarly
> to qobject_input_visitor, takes an input QObject that will dictate
> which members get freed and which don't. Members not present in the
> input QObject will be left unchanged in the visited QAPI object.
> 
> This is useful to free memory just before perfoming a visit with
> qobject_input_visitor on a pre-existing, non-null QAPI object. If the
> same QObject is passed to both visitors, the pointers overwritten by
> the input visitor match the ones that are freed by the dealloc
> visitor.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  include/qapi/dealloc-visitor.h |   6 ++
>  qapi/qapi-dealloc-visitor.c    | 173 ++++++++++++++++++++++++++++++++-

First of all, I saw that all prior visitors should always have unit tests
under tests/unit/.  Maybe we should also attach an unit test?

>  2 files changed, 178 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
> index c36715fdf3..96c7bf35c3 100644
> --- a/include/qapi/dealloc-visitor.h
> +++ b/include/qapi/dealloc-visitor.h
> @@ -25,4 +25,10 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
>   */
>  Visitor *qapi_dealloc_visitor_new(void);
>  
> +/*
> + * Like qapi_dealloc_visitor_new but visits a QObject and only frees
> + * present members.
> + */
> +Visitor *qapi_dealloc_present_visitor_new(QObject *);
> +
>  #endif
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 57a2c904bb..90b017cc93 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -14,14 +14,146 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/dealloc-visitor.h"
> +#include "qemu/queue.h"
> +#include "qobject/qdict.h"
> +#include "qobject/qlist.h"
>  #include "qobject/qnull.h"
>  #include "qapi/visitor-impl.h"
>  
> +typedef struct QStackEntry {
> +    QObject *obj;               /* QDict or QList being visited */
> +    void *qapi;

This one is only for debugging purpose, but not required, right?

I wonder if we could just rely on the unit test for correctness, otherwise
we kind of run some testing code with/without --enable-debug.  Not a huge
deal I think.. so see this a pure question.

> +    const QListEntry *entry;    /* If @obj is QList: unvisited tail */
> +    QSLIST_ENTRY(QStackEntry) node;
> +} QStackEntry;
> +
>  struct QapiDeallocVisitor
>  {
>      Visitor visitor;
> +    QObject *root;
> +    QSLIST_HEAD(, QStackEntry) stack;
>  };
>  
> +static void qapi_dealloc_pop(Visitor *v, void **obj)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> +    assert(se && se->qapi == obj);
> +    QSLIST_REMOVE_HEAD(&qdv->stack, node);
> +    g_free(se);
> +}
> +
> +static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +    QStackEntry *se = g_new0(QStackEntry, 1);
> +
> +    assert(obj);
> +    se->obj = obj;
> +    se->qapi = qapi;
> +
> +    if (qobject_type(obj) == QTYPE_QLIST) {
> +        se->entry = qlist_first(qobject_to(QList, obj));

I still don't yet understand why do we care about lists here.

I'm trying to guess, what this code wanted to do: we pushed the 1st entry
of the list into the stack, trying to make it as a reference for all the
items later within the list.

But IIUC we can still define the conditiona-dealloc visitor to be even
simpler, right?  Say, if the ref qobject has the qlist object, then free
the whole list?

IMHO there're three things we need to manage on conditional deallocations:
(1) struct, (2) list, (3) alternatives.  All the rest seem to be scalars.

So I wonder if we could define this visitor, so that it treats both (2) and
(3) the same way (to always dealloc as long as present).

That sounds at least making more sense when I picture that in migration
parameters: if we have a list in the parameters (e.g. cpr-exec-command), as
long as the list is present in the ref, we should free the whole thing
completely on the other one being visited.

> +    }
> +
> +    QSLIST_INSERT_HEAD(&qdv->stack, se, node);
> +}
> +
> +static QObject *qapi_dealloc_try_get_object(QapiDeallocVisitor *qdv, const char *name)
> +{
> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +    QObject *qobj;
> +    QObject *ret = NULL;
> +
> +    if (!se) {
> +        assert(qdv->root);
> +        return qdv->root;
> +    }
> +
> +    qobj = se->obj;
> +    assert(qobj);
> +
> +    if (qobject_type(qobj) == QTYPE_QDICT) {
> +        assert(name);
> +        ret = qdict_get(qobject_to(QDict, qobj), name);
> +    } else {
> +        assert(qobject_type(qobj) == QTYPE_QLIST);
> +        assert(!name);
> +        if (se->entry) {
> +            ret = qlist_entry_obj(se->entry);
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static bool qapi_dealloc_present_start_struct(Visitor *v, const char *name,
> +                                              void **obj, size_t size,
> +                                              Error **errp)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
> +
> +    if (!qobj) {
> +        return false;
> +    }
> +    assert(qobject_type(qobj) == QTYPE_QDICT);
> +    qapi_dealloc_push(v, qobj, obj);
> +    return true;
> +}
> +
> +static void qapi_dealloc_present_end_struct(Visitor *v, void **obj)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> +    assert(qobject_type(se->obj) == QTYPE_QDICT);
> +    qapi_dealloc_pop(v, obj);
> +
> +    if (obj) {
> +        g_free(*obj);
> +    }
> +}
> +
> +static bool qapi_dealloc_present_start_list(Visitor *v, const char *name,
> +                                            GenericList **list, size_t size,
> +                                            Error **errp)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
> +
> +    if (!qobj) {
> +        return false;
> +    }
> +    assert(qobject_type(qobj) == QTYPE_QLIST);
> +    qapi_dealloc_push(v, qobj, list);
> +    return true;
> +}
> +
> +static void qapi_dealloc_present_end_list(Visitor *v, void **obj)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> +    assert(qobject_type(se->obj) == QTYPE_QLIST);
> +    qapi_dealloc_pop(v, obj);
> +}
> +
> +static void qapi_dealloc_present_free(Visitor *v)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +
> +    while (!QSLIST_EMPTY(&qdv->stack)) {
> +        QStackEntry *se = QSLIST_FIRST(&qdv->stack);
> +
> +        QSLIST_REMOVE_HEAD(&qdv->stack, node);
> +        g_free(se);
> +    }
> +    qobject_unref(qdv->root);
> +    g_free(qdv);
> +}
> +
>  static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
>                                        size_t unused, Error **errp)
>  {
> @@ -35,6 +167,21 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
>      }
>  }
>  
> +static bool qapi_dealloc_start_alternate(Visitor *v, const char *name,
> +                                         GenericAlternate **obj, size_t size,
> +                                         Error **errp)
> +{
> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> +    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
> +
> +    if (!qobj) {
> +        return false;
> +    }
> +    assert(*obj);
> +    (*obj)->type = qobject_type(qobj);
> +    return true;
> +}
> +
>  static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
>  {
>      if (obj) {
> @@ -117,13 +264,14 @@ static void qapi_dealloc_free(Visitor *v)
>      g_free(container_of(v, QapiDeallocVisitor, visitor));
>  }
>  
> -Visitor *qapi_dealloc_visitor_new(void)
> +static QapiDeallocVisitor *qapi_dealloc_visitor_new_base(void)
>  {
>      QapiDeallocVisitor *v;
>  
>      v = g_malloc0(sizeof(*v));
>  
>      v->visitor.type = VISITOR_DEALLOC;
> +
>      v->visitor.start_struct = qapi_dealloc_start_struct;
>      v->visitor.end_struct = qapi_dealloc_end_struct;
>      v->visitor.end_alternate = qapi_dealloc_end_alternate;
> @@ -139,5 +287,28 @@ Visitor *qapi_dealloc_visitor_new(void)
>      v->visitor.type_null = qapi_dealloc_type_null;
>      v->visitor.free = qapi_dealloc_free;

IMHO, setting the hooks once then overwrite, is less clean than moving them
into qapi_dealloc_visitor_new() if dealloc_present visitor will do that.

>  
> +    return v;
> +}
> +
> +Visitor *qapi_dealloc_visitor_new(void)
> +{
> +    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
> +
> +    return &v->visitor;
> +}
> +
> +Visitor *qapi_dealloc_present_visitor_new(QObject *obj)
> +{
> +    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
> +
> +    v->visitor.start_alternate = qapi_dealloc_start_alternate;
> +    v->visitor.start_list = qapi_dealloc_present_start_list;
> +    v->visitor.end_list = qapi_dealloc_present_end_list;
> +    v->visitor.start_struct = qapi_dealloc_present_start_struct;
> +    v->visitor.end_struct = qapi_dealloc_present_end_struct;
> +    v->visitor.free = qapi_dealloc_present_free;
> +
> +    v->root = qobject_ref(obj);
> +
>      return &v->visitor;
>  }
> -- 
> 2.51.0
> 

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Mon, Feb 02, 2026 at 07:40:56PM -0300, Fabiano Rosas wrote:
>> Implement a visitor that frees the pointer members of the visited QAPI
>> object in the same way that qapi_dealloc_visitor does, but similarly
>> to qobject_input_visitor, takes an input QObject that will dictate
>> which members get freed and which don't. Members not present in the
>> input QObject will be left unchanged in the visited QAPI object.
>> 
>> This is useful to free memory just before perfoming a visit with
>> qobject_input_visitor on a pre-existing, non-null QAPI object. If the
>> same QObject is passed to both visitors, the pointers overwritten by
>> the input visitor match the ones that are freed by the dealloc
>> visitor.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  include/qapi/dealloc-visitor.h |   6 ++
>>  qapi/qapi-dealloc-visitor.c    | 173 ++++++++++++++++++++++++++++++++-
>
> First of all, I saw that all prior visitors should always have unit tests
> under tests/unit/.  Maybe we should also attach an unit test?
>

Yep.

>>  2 files changed, 178 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
>> index c36715fdf3..96c7bf35c3 100644
>> --- a/include/qapi/dealloc-visitor.h
>> +++ b/include/qapi/dealloc-visitor.h
>> @@ -25,4 +25,10 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
>>   */
>>  Visitor *qapi_dealloc_visitor_new(void);
>>  
>> +/*
>> + * Like qapi_dealloc_visitor_new but visits a QObject and only frees
>> + * present members.
>> + */
>> +Visitor *qapi_dealloc_present_visitor_new(QObject *);
>> +
>>  #endif
>> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
>> index 57a2c904bb..90b017cc93 100644
>> --- a/qapi/qapi-dealloc-visitor.c
>> +++ b/qapi/qapi-dealloc-visitor.c
>> @@ -14,14 +14,146 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "qapi/dealloc-visitor.h"
>> +#include "qemu/queue.h"
>> +#include "qobject/qdict.h"
>> +#include "qobject/qlist.h"
>>  #include "qobject/qnull.h"
>>  #include "qapi/visitor-impl.h"
>>  
>> +typedef struct QStackEntry {
>> +    QObject *obj;               /* QDict or QList being visited */
>> +    void *qapi;
>
> This one is only for debugging purpose, but not required, right?
>
> I wonder if we could just rely on the unit test for correctness, otherwise
> we kind of run some testing code with/without --enable-debug.  Not a huge
> deal I think.. so see this a pure question.
>

I guess I can drop it. Should be ok.

>> +    const QListEntry *entry;    /* If @obj is QList: unvisited tail */
>> +    QSLIST_ENTRY(QStackEntry) node;
>> +} QStackEntry;
>> +
>>  struct QapiDeallocVisitor
>>  {
>>      Visitor visitor;
>> +    QObject *root;
>> +    QSLIST_HEAD(, QStackEntry) stack;
>>  };
>>  
>> +static void qapi_dealloc_pop(Visitor *v, void **obj)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> +    assert(se && se->qapi == obj);
>> +    QSLIST_REMOVE_HEAD(&qdv->stack, node);
>> +    g_free(se);
>> +}
>> +
>> +static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +    QStackEntry *se = g_new0(QStackEntry, 1);
>> +
>> +    assert(obj);
>> +    se->obj = obj;
>> +    se->qapi = qapi;
>> +
>> +    if (qobject_type(obj) == QTYPE_QLIST) {
>> +        se->entry = qlist_first(qobject_to(QList, obj));
>
> I still don't yet understand why do we care about lists here.
>
> I'm trying to guess, what this code wanted to do: we pushed the 1st entry
> of the list into the stack, trying to make it as a reference for all the
> items later within the list.
>
> But IIUC we can still define the conditiona-dealloc visitor to be even
> simpler, right?  Say, if the ref qobject has the qlist object, then free
> the whole list?
>

I need to look at this closely, but I think we need to hold the list ref
so we can walk each of the objects inside of it and free them, then free
the empty list. When you "free the whole list" that would mean
qapi_free_<something>, which we can't do here, being already inside
visitor code; and free(list) doesn't free it's children, of course.

> IMHO there're three things we need to manage on conditional deallocations:
> (1) struct, (2) list, (3) alternatives.  All the rest seem to be scalars.
>
> So I wonder if we could define this visitor, so that it treats both (2) and
> (3) the same way (to always dealloc as long as present).
>
> That sounds at least making more sense when I picture that in migration
> parameters: if we have a list in the parameters (e.g. cpr-exec-command), as
> long as the list is present in the ref, we should free the whole thing
> completely on the other one being visited.
>

Yes, that makes sense. I'm not expecting individual list items to be
ever updated separately. However, as I said above, I'm not sure whether
we can free the whole list without some sort of a walk of its
members. I'll check and get back to you.

>> +    }
>> +
>> +    QSLIST_INSERT_HEAD(&qdv->stack, se, node);
>> +}
>> +
>> +static QObject *qapi_dealloc_try_get_object(QapiDeallocVisitor *qdv, const char *name)
>> +{
>> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +    QObject *qobj;
>> +    QObject *ret = NULL;
>> +
>> +    if (!se) {
>> +        assert(qdv->root);
>> +        return qdv->root;
>> +    }
>> +
>> +    qobj = se->obj;
>> +    assert(qobj);
>> +
>> +    if (qobject_type(qobj) == QTYPE_QDICT) {
>> +        assert(name);
>> +        ret = qdict_get(qobject_to(QDict, qobj), name);
>> +    } else {
>> +        assert(qobject_type(qobj) == QTYPE_QLIST);
>> +        assert(!name);
>> +        if (se->entry) {
>> +            ret = qlist_entry_obj(se->entry);
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static bool qapi_dealloc_present_start_struct(Visitor *v, const char *name,
>> +                                              void **obj, size_t size,
>> +                                              Error **errp)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
>> +
>> +    if (!qobj) {
>> +        return false;
>> +    }
>> +    assert(qobject_type(qobj) == QTYPE_QDICT);
>> +    qapi_dealloc_push(v, qobj, obj);
>> +    return true;
>> +}
>> +
>> +static void qapi_dealloc_present_end_struct(Visitor *v, void **obj)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> +    assert(qobject_type(se->obj) == QTYPE_QDICT);
>> +    qapi_dealloc_pop(v, obj);
>> +
>> +    if (obj) {
>> +        g_free(*obj);
>> +    }
>> +}
>> +
>> +static bool qapi_dealloc_present_start_list(Visitor *v, const char *name,
>> +                                            GenericList **list, size_t size,
>> +                                            Error **errp)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
>> +
>> +    if (!qobj) {
>> +        return false;
>> +    }
>> +    assert(qobject_type(qobj) == QTYPE_QLIST);
>> +    qapi_dealloc_push(v, qobj, list);
>> +    return true;
>> +}
>> +
>> +static void qapi_dealloc_present_end_list(Visitor *v, void **obj)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +    QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> +    assert(qobject_type(se->obj) == QTYPE_QLIST);
>> +    qapi_dealloc_pop(v, obj);
>> +}
>> +
>> +static void qapi_dealloc_present_free(Visitor *v)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +
>> +    while (!QSLIST_EMPTY(&qdv->stack)) {
>> +        QStackEntry *se = QSLIST_FIRST(&qdv->stack);
>> +
>> +        QSLIST_REMOVE_HEAD(&qdv->stack, node);
>> +        g_free(se);
>> +    }
>> +    qobject_unref(qdv->root);
>> +    g_free(qdv);
>> +}
>> +
>>  static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
>>                                        size_t unused, Error **errp)
>>  {
>> @@ -35,6 +167,21 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
>>      }
>>  }
>>  
>> +static bool qapi_dealloc_start_alternate(Visitor *v, const char *name,
>> +                                         GenericAlternate **obj, size_t size,
>> +                                         Error **errp)
>> +{
>> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
>> +    QObject *qobj = qapi_dealloc_try_get_object(qdv, name);
>> +
>> +    if (!qobj) {
>> +        return false;
>> +    }
>> +    assert(*obj);
>> +    (*obj)->type = qobject_type(qobj);
>> +    return true;
>> +}
>> +
>>  static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
>>  {
>>      if (obj) {
>> @@ -117,13 +264,14 @@ static void qapi_dealloc_free(Visitor *v)
>>      g_free(container_of(v, QapiDeallocVisitor, visitor));
>>  }
>>  
>> -Visitor *qapi_dealloc_visitor_new(void)
>> +static QapiDeallocVisitor *qapi_dealloc_visitor_new_base(void)
>>  {
>>      QapiDeallocVisitor *v;
>>  
>>      v = g_malloc0(sizeof(*v));
>>  
>>      v->visitor.type = VISITOR_DEALLOC;
>> +
>>      v->visitor.start_struct = qapi_dealloc_start_struct;
>>      v->visitor.end_struct = qapi_dealloc_end_struct;
>>      v->visitor.end_alternate = qapi_dealloc_end_alternate;
>> @@ -139,5 +287,28 @@ Visitor *qapi_dealloc_visitor_new(void)
>>      v->visitor.type_null = qapi_dealloc_type_null;
>>      v->visitor.free = qapi_dealloc_free;
>
> IMHO, setting the hooks once then overwrite, is less clean than moving them
> into qapi_dealloc_visitor_new() if dealloc_present visitor will do that.
>

ok

>>  
>> +    return v;
>> +}
>> +
>> +Visitor *qapi_dealloc_visitor_new(void)
>> +{
>> +    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
>> +
>> +    return &v->visitor;
>> +}
>> +
>> +Visitor *qapi_dealloc_present_visitor_new(QObject *obj)
>> +{
>> +    QapiDeallocVisitor *v = qapi_dealloc_visitor_new_base();
>> +
>> +    v->visitor.start_alternate = qapi_dealloc_start_alternate;
>> +    v->visitor.start_list = qapi_dealloc_present_start_list;
>> +    v->visitor.end_list = qapi_dealloc_present_end_list;
>> +    v->visitor.start_struct = qapi_dealloc_present_start_struct;
>> +    v->visitor.end_struct = qapi_dealloc_present_end_struct;
>> +    v->visitor.free = qapi_dealloc_present_free;
>> +
>> +    v->root = qobject_ref(obj);
>> +
>>      return &v->visitor;
>>  }
>> -- 
>> 2.51.0
>>

On Fri, Feb 06, 2026 at 09:19:21AM -0300, Fabiano Rosas wrote:
> >> +static void qapi_dealloc_push(Visitor *v, QObject *obj, void *qapi)
> >> +{
> >> +    QapiDeallocVisitor *qdv = container_of(v, QapiDeallocVisitor, visitor);
> >> +    QStackEntry *se = g_new0(QStackEntry, 1);
> >> +
> >> +    assert(obj);
> >> +    se->obj = obj;
> >> +    se->qapi = qapi;
> >> +
> >> +    if (qobject_type(obj) == QTYPE_QLIST) {
> >> +        se->entry = qlist_first(qobject_to(QList, obj));
> >
> > I still don't yet understand why do we care about lists here.
> >
> > I'm trying to guess, what this code wanted to do: we pushed the 1st entry
> > of the list into the stack, trying to make it as a reference for all the
> > items later within the list.
> >
> > But IIUC we can still define the conditiona-dealloc visitor to be even
> > simpler, right?  Say, if the ref qobject has the qlist object, then free
> > the whole list?
> >
> 
> I need to look at this closely, but I think we need to hold the list ref
> so we can walk each of the objects inside of it and free them, then free
> the empty list. When you "free the whole list" that would mean
> qapi_free_<something>, which we can't do here, being already inside
> visitor code; and free(list) doesn't free it's children, of course.

True.

> 
> > IMHO there're three things we need to manage on conditional deallocations:
> > (1) struct, (2) list, (3) alternatives.  All the rest seem to be scalars.
> >
> > So I wonder if we could define this visitor, so that it treats both (2) and
> > (3) the same way (to always dealloc as long as present).
> >
> > That sounds at least making more sense when I picture that in migration
> > parameters: if we have a list in the parameters (e.g. cpr-exec-command), as
> > long as the list is present in the ref, we should free the whole thing
> > completely on the other one being visited.
> >
> 
> Yes, that makes sense. I'm not expecting individual list items to be
> ever updated separately. However, as I said above, I'm not sure whether
> we can free the whole list without some sort of a walk of its
> members. I'll check and get back to you.

My QAPI kongfu is very limited.. but my current understanding is the
visitor framework will make sure everything will be visited properly within
the list, and after each visit to the list entry, it'll finally free the
list structure (in case of dealloc visitor) via qapi_dealloc_next_list().
I believe that's also what the new dealloc_present visitor should rely on.

So my understanding is maybe we don't need to hold any reference to the
list being walked.  However maybe we want to remember we're walking this
list, then no matter how deep we walk into this list, we should stop
checking / referencing the ref object but just always free everything (aka,
fallback to the basic dealloc operations).

When thinking about that, I found maybe it's not that list is special; it's
when the "top level" is special.

Consider if we have a migration parameter that is a nested struct:

  - struct1
    - struct2
      - int
    - struct3
      - boolean

Then if currently we have this value for this parameter:

  - struct1
    - struct2
      - int=1
    - struct3
      - string="foobar"

When we take an update from the user on this specific parameter, and if the
user's input is:

  - struct1
    - struct2
      - int=2
    (omit struct3)

With your current patch, you'll leave struct3 and the sub-field string
untouched.  However, IIUC the current semantics (of migrate_set_parameters,
at least) should be that we use the new data to fully replace the old
struct1 tree, making sure struct3 alone with the string to be unset?

So I wonder if we should just remember the top of the stack (which must be
a qdict), then free anything below that whenever the top element is present
in the ref.

-- 
Peter Xu