[PATCH for-5.1 0/5] qom: HMP command fixes

[PATCH for-5.1 0/5] qom: HMP command fixes

[Markus Armbruster]

A leak fix (recent regression), a help fix (regression in 5.0), a bit
of performance work, and a doc fix.  The leak fix looks like a must
have for 5.1.

Markus Armbruster (5):
  qdev: Fix device_add DRIVER,help to print to monitor
  qom: Plug memory leak in "info qom-tree"
  qom: Change object_get_canonical_path_component() not to malloc
  qom: Document object_get_canonical_path() returns malloced string
  qom: Make info qom-tree sort children more efficiently

 include/qom/object.h       |  7 ++++---
 backends/hostmem.c         |  2 +-
 block/throttle-groups.c    |  2 +-
 gdbstub.c                  |  2 +-
 hw/arm/xlnx-zynqmp.c       |  6 ++----
 hw/block/nvme.c            |  5 ++---
 hw/core/machine-qmp-cmds.c |  2 +-
 hw/core/machine.c          |  5 ++---
 hw/mem/nvdimm.c            |  5 ++---
 hw/mem/pc-dimm.c           |  5 ++---
 hw/misc/ivshmem.c          |  5 ++---
 hw/ppc/spapr_drc.c         |  3 +--
 hw/virtio/virtio-crypto.c  |  5 ++---
 hw/virtio/virtio-mem.c     |  6 ++----
 hw/virtio/virtio-pmem.c    |  5 ++---
 iothread.c                 |  9 ++++-----
 net/net.c                  |  6 ++----
 qdev-monitor.c             |  2 +-
 qom/object.c               |  7 +++----
 qom/qom-hmp-cmds.c         | 30 +++++++++++++++---------------
 scsi/pr-manager-helper.c   |  3 +--
 scsi/pr-manager.c          |  2 +-
 softmmu/memory.c           |  2 +-
 hw/ppc/trace-events        |  2 +-
 24 files changed, 56 insertions(+), 72 deletions(-)

-- 
2.26.2

[PATCH for-5.1 1/5] qdev: Fix device_add DRIVER, help to print to monitor

[Markus Armbruster]

Help on device properties gets printed to stdout instead of the
monitor.  If you have the monitor anywhere else, no help for you.
Broken when commit e1043d674d "qdev: use object_property_help()"
accidentally switched from qemu_printf() to printf().  Switch right
back.

Fixes: e1043d674d792ff64aebae1a3eafc08b38a8a085
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qdev-monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 71ebce19df..e9b7228480 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -300,7 +300,7 @@ int qdev_device_help(QemuOpts *opts)
     }
     g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
     for (i = 0; i < array->len; i++) {
-        printf("%s\n", (char *)array->pdata[i]);
+        qemu_printf("%s\n", (char *)array->pdata[i]);
     }
     g_ptr_array_set_free_func(array, g_free);
     g_ptr_array_free(array, true);
-- 
2.26.2

Re: [PATCH for-5.1 1/5] qdev: Fix device_add DRIVER, help to print to monitor

[Philippe Mathieu-Daudé]

On 7/14/20 6:01 PM, Markus Armbruster wrote:
> Help on device properties gets printed to stdout instead of the
> monitor.  If you have the monitor anywhere else, no help for you.
> Broken when commit e1043d674d "qdev: use object_property_help()"
> accidentally switched from qemu_printf() to printf().  Switch right
> back.
> 
> Fixes: e1043d674d792ff64aebae1a3eafc08b38a8a085
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 71ebce19df..e9b7228480 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c

#pragma GCC poison printf
#pragma GCC poison fprintf

;)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> @@ -300,7 +300,7 @@ int qdev_device_help(QemuOpts *opts)
>      }
>      g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
>      for (i = 0; i < array->len; i++) {
> -        printf("%s\n", (char *)array->pdata[i]);
> +        qemu_printf("%s\n", (char *)array->pdata[i]);
>      }
>      g_ptr_array_set_free_func(array, g_free);
>      g_ptr_array_free(array, true);
> 

Re: [PATCH for-5.1 1/5] qdev: Fix device_add DRIVER, help to print to monitor

[Li Qiang]

Markus Armbruster <armbru@redhat.com> 于2020年7月15日周三 上午12:09写道：
>
> Help on device properties gets printed to stdout instead of the
> monitor.  If you have the monitor anywhere else, no help for you.
> Broken when commit e1043d674d "qdev: use object_property_help()"
> accidentally switched from qemu_printf() to printf().  Switch right
> back.
>
> Fixes: e1043d674d792ff64aebae1a3eafc08b38a8a085
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 71ebce19df..e9b7228480 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -300,7 +300,7 @@ int qdev_device_help(QemuOpts *opts)
>      }
>      g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
>      for (i = 0; i < array->len; i++) {
> -        printf("%s\n", (char *)array->pdata[i]);
> +        qemu_printf("%s\n", (char *)array->pdata[i]);
>      }
>      g_ptr_array_set_free_func(array, g_free);
>      g_ptr_array_free(array, true);
> --
> 2.26.2
>
>

Re: [PATCH for-5.1 1/5] qdev: Fix device_add DRIVER, help to print to monitor

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Help on device properties gets printed to stdout instead of the
> monitor.  If you have the monitor anywhere else, no help for you.
> Broken when commit e1043d674d "qdev: use object_property_help()"
> accidentally switched from qemu_printf() to printf().  Switch right
> back.
> 
> Fixes: e1043d674d792ff64aebae1a3eafc08b38a8a085
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>



Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 71ebce19df..e9b7228480 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -300,7 +300,7 @@ int qdev_device_help(QemuOpts *opts)
>      }
>      g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
>      for (i = 0; i < array->len; i++) {
> -        printf("%s\n", (char *)array->pdata[i]);
> +        qemu_printf("%s\n", (char *)array->pdata[i]);
>      }
>      g_ptr_array_set_free_func(array, g_free);
>      g_ptr_array_free(array, true);
> -- 
> 2.26.2
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH for-5.1 2/5] qom: Plug memory leak in "info qom-tree"

[Markus Armbruster]

Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
created a memory leak, because I didn't realize
object_get_canonical_path_component()'s value needs to be freed.

Reproducer:

    $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
    QEMU 5.0.50 monitor - type 'help' for more information
    (qemu) info qom-tree

This leaks some 4500 path components, 12-13 characters on average,
i.e. roughly 100kBytes depending on the allocator.  A couple of
hundred "info qom-tree" here, a couple of hundred there, and soon
enough we're talking about real memory.

Plug the leak.

Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qom/qom-hmp-cmds.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index 9ed8bb1c9f..aaacadacca 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
 
 static int qom_composition_compare(const void *a, const void *b, void *ignore)
 {
-    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
-                     b ? object_get_canonical_path_component(b) : NULL);
+    g_autofree char *ac = object_get_canonical_path_component(a);
+    g_autofree char *bc = object_get_canonical_path_component(b);
+
+    return g_strcmp0(ac, bc);
 }
 
 static int insert_qom_composition_child(Object *obj, void *opaque)
-- 
2.26.2

Re: [PATCH for-5.1 2/5] qom: Plug memory leak in "info qom-tree"

[Philippe Mathieu-Daudé]

On 7/14/20 6:01 PM, Markus Armbruster wrote:
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> created a memory leak, because I didn't realize
> object_get_canonical_path_component()'s value needs to be freed.
> 
> Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>     QEMU 5.0.50 monitor - type 'help' for more information
>     (qemu) info qom-tree
> 
> This leaks some 4500 path components, 12-13 characters on average,
> i.e. roughly 100kBytes depending on the allocator.  A couple of
> hundred "info qom-tree" here, a couple of hundred there, and soon
> enough we're talking about real memory.
> 
> Plug the leak.
> 
> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qom/qom-hmp-cmds.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 9ed8bb1c9f..aaacadacca 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>  
>  static int qom_composition_compare(const void *a, const void *b, void *ignore)
>  {
> -    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
> -                     b ? object_get_canonical_path_component(b) : NULL);
> +    g_autofree char *ac = object_get_canonical_path_component(a);
> +    g_autofree char *bc = object_get_canonical_path_component(b);
> +
> +    return g_strcmp0(ac, bc);
>  }
>  
>  static int insert_qom_composition_child(Object *obj, void *opaque)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-5.1 2/5] qom: Plug memory leak in "info qom-tree"

[Philippe Mathieu-Daudé]

On 7/14/20 6:01 PM, Markus Armbruster wrote:
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> created a memory leak, because I didn't realize
> object_get_canonical_path_component()'s value needs to be freed.
> 
> Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>     QEMU 5.0.50 monitor - type 'help' for more information
>     (qemu) info qom-tree
> 
> This leaks some 4500 path components, 12-13 characters on average,
> i.e. roughly 100kBytes depending on the allocator.  A couple of
> hundred "info qom-tree" here, a couple of hundred there, and soon
> enough we're talking about real memory.
> 
> Plug the leak.
> 
> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qom/qom-hmp-cmds.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 9ed8bb1c9f..aaacadacca 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>  
>  static int qom_composition_compare(const void *a, const void *b, void *ignore)
>  {
> -    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
> -                     b ? object_get_canonical_path_component(b) : NULL);
> +    g_autofree char *ac = object_get_canonical_path_component(a);
> +    g_autofree char *bc = object_get_canonical_path_component(b);
> +
> +    return g_strcmp0(ac, bc);
>  }
>  
>  static int insert_qom_composition_child(Object *obj, void *opaque)
> 

Li Qiang set this too:
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04732.html

Re: [PATCH for-5.1 2/5] qom: Plug memory leak in "info qom-tree"

[Li Qiang]

Markus Armbruster <armbru@redhat.com> 于2020年7月15日周三 上午12:05写道：
>
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> created a memory leak, because I didn't realize
> object_get_canonical_path_component()'s value needs to be freed.
>
> Reproducer:
>
>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>     QEMU 5.0.50 monitor - type 'help' for more information
>     (qemu) info qom-tree
>
> This leaks some 4500 path components, 12-13 characters on average,
> i.e. roughly 100kBytes depending on the allocator.  A couple of
> hundred "info qom-tree" here, a couple of hundred there, and soon
> enough we're talking about real memory.
>
> Plug the leak.
>
> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

I have also send out this patch.
I hope the maintainer will pick your patch.

Thanks,
Li Qiang

> ---
>  qom/qom-hmp-cmds.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 9ed8bb1c9f..aaacadacca 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>
>  static int qom_composition_compare(const void *a, const void *b, void *ignore)
>  {
> -    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
> -                     b ? object_get_canonical_path_component(b) : NULL);
> +    g_autofree char *ac = object_get_canonical_path_component(a);
> +    g_autofree char *bc = object_get_canonical_path_component(b);
> +
> +    return g_strcmp0(ac, bc);
>  }
>
>  static int insert_qom_composition_child(Object *obj, void *opaque)
> --
> 2.26.2
>
>

Re: [PATCH for-5.1 2/5] qom: Plug memory leak in "info qom-tree"

[Thomas Huth]

On 15/07/2020 17.19, Li Qiang wrote:
> Markus Armbruster <armbru@redhat.com> 于2020年7月15日周三 上午12:05写道：
>>
>> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
>> created a memory leak, because I didn't realize
>> object_get_canonical_path_component()'s value needs to be freed.
>>
>> Reproducer:
>>
>>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>>     QEMU 5.0.50 monitor - type 'help' for more information
>>     (qemu) info qom-tree
>>
>> This leaks some 4500 path components, 12-13 characters on average,
>> i.e. roughly 100kBytes depending on the allocator.  A couple of
>> hundred "info qom-tree" here, a couple of hundred there, and soon
>> enough we're talking about real memory.
>>
>> Plug the leak.
>>
>> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Tested-by: Thomas Huth <thuth@redhat.com>


> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> 
> I have also send out this patch.
> I hope the maintainer will pick your patch.

Thanks a lot for your patch, too! Normally, I'd say "first come, first
serve" and suggest that we use your patch, but since Markus' patch
series has some more patches which would break due to the contextual
differences, I think it's slightly better this time to use Markus'
version of the patch.

 Regards,
  Thomas

[PATCH for-5.1 3/5] qom: Change object_get_canonical_path_component() not to malloc

[Markus Armbruster]

object_get_canonical_path_component() returns a malloced copy of a
property name on success, null on failure.

19 of its 25 callers immediately free the returned copy.

Change object_get_canonical_path_component() to return the property
name directly.  Since modifying the name would be wrong, adjust the
return type to const char *.

Drop the free from the 19 callers become simpler, add the g_strdup()
to the other six.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qom/object.h       |  2 +-
 backends/hostmem.c         |  2 +-
 block/throttle-groups.c    |  2 +-
 gdbstub.c                  |  2 +-
 hw/arm/xlnx-zynqmp.c       |  6 ++----
 hw/block/nvme.c            |  5 ++---
 hw/core/machine-qmp-cmds.c |  2 +-
 hw/core/machine.c          |  5 ++---
 hw/mem/nvdimm.c            |  5 ++---
 hw/mem/pc-dimm.c           |  5 ++---
 hw/misc/ivshmem.c          |  5 ++---
 hw/ppc/spapr_drc.c         |  3 +--
 hw/virtio/virtio-crypto.c  |  5 ++---
 hw/virtio/virtio-mem.c     |  6 ++----
 hw/virtio/virtio-pmem.c    |  5 ++---
 iothread.c                 |  9 ++++-----
 net/net.c                  |  6 ++----
 qom/object.c               |  7 +++----
 qom/qom-hmp-cmds.c         | 11 ++++-------
 scsi/pr-manager-helper.c   |  3 +--
 scsi/pr-manager.c          |  2 +-
 softmmu/memory.c           |  2 +-
 hw/ppc/trace-events        |  2 +-
 23 files changed, 41 insertions(+), 61 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 79c8f838b6..55d925d2c8 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1462,7 +1462,7 @@ Object *object_get_internal_root(void);
  * path is the path within the composition tree starting from the root.
  * %NULL if the object doesn't have a parent (and thus a canonical path).
  */
-char *object_get_canonical_path_component(const Object *obj);
+const char *object_get_canonical_path_component(const Object *obj);
 
 /**
  * object_get_canonical_path:
diff --git a/backends/hostmem.c b/backends/hostmem.c
index c614f1bdc1..4bde00e8e7 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -33,7 +33,7 @@ char *
 host_memory_backend_get_name(HostMemoryBackend *backend)
 {
     if (!backend->use_canonical_path) {
-        return object_get_canonical_path_component(OBJECT(backend));
+        return g_strdup(object_get_canonical_path_component(OBJECT(backend)));
     }
 
     return object_get_canonical_path(OBJECT(backend));
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 03a53c89ea..98fea7fd47 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -771,7 +771,7 @@ static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
 
     /* set group name to object id if it exists */
     if (!tg->name && tg->parent_obj.parent) {
-        tg->name = object_get_canonical_path_component(OBJECT(obj));
+        tg->name = g_strdup(object_get_canonical_path_component(OBJECT(obj)));
     }
     /* We must have a group name at this point */
     assert(tg->name);
diff --git a/gdbstub.c b/gdbstub.c
index 6950fd243f..f3a318cd7f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2059,7 +2059,7 @@ static void handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
         /* Print the CPU model and name in multiprocess mode */
         ObjectClass *oc = object_get_class(OBJECT(cpu));
         const char *cpu_model = object_class_get_name(oc);
-        g_autofree char *cpu_name =
+        const char *cpu_name =
             object_get_canonical_path_component(OBJECT(cpu));
         g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
                         cpu->halted ? "halted " : "running");
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 772cfa3771..e14323c991 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -190,7 +190,7 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
     qdev_prop_set_uint32(DEVICE(&s->rpu_cluster), "cluster-id", 1);
 
     for (i = 0; i < num_rpus; i++) {
-        char *name;
+        const char *name;
 
         object_initialize_child(OBJECT(&s->rpu_cluster), "rpu-cpu[*]",
                                 &s->rpu_cpu[i],
@@ -204,7 +204,6 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
         } else {
             s->boot_cpu_ptr = &s->rpu_cpu[i];
         }
-        g_free(name);
 
         object_property_set_bool(OBJECT(&s->rpu_cpu[i]), "reset-hivecs", true,
                                  &error_abort);
@@ -341,7 +340,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
 
     /* Realize APUs before realizing the GIC. KVM requires this.  */
     for (i = 0; i < num_apus; i++) {
-        char *name;
+        const char *name;
 
         object_property_set_int(OBJECT(&s->apu_cpu[i]), "psci-conduit",
                                 QEMU_PSCI_CONDUIT_SMC, &error_abort);
@@ -354,7 +353,6 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         } else {
             s->boot_cpu_ptr = &s->apu_cpu[i];
         }
-        g_free(name);
 
         object_property_set_bool(OBJECT(&s->apu_cpu[i]), "has_el3", s->secure,
                                  NULL);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1aee042d4c..3426e17e65 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1397,9 +1397,8 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
 
     if (!n->params.cmb_size_mb && n->pmrdev) {
         if (host_memory_backend_is_mapped(n->pmrdev)) {
-            char *path = object_get_canonical_path_component(OBJECT(n->pmrdev));
-            error_setg(errp, "can't use already busy memdev: %s", path);
-            g_free(path);
+            error_setg(errp, "can't use already busy memdev: %s",
+                       object_get_canonical_path_component(OBJECT(n->pmrdev)));
             return;
         }
 
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 2c5da8413d..963088b798 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -315,7 +315,7 @@ static int query_memdev(Object *obj, void *opaque)
 
         m->value = g_malloc0(sizeof(*m->value));
 
-        m->value->id = object_get_canonical_path_component(obj);
+        m->value->id = g_strdup(object_get_canonical_path_component(obj));
         m->value->has_id = !!m->value->id;
 
         m->value->size = object_property_get_uint(obj, "size",
diff --git a/hw/core/machine.c b/hw/core/machine.c
index eb267b828d..2f881d6d75 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1073,9 +1073,8 @@ MemoryRegion *machine_consume_memdev(MachineState *machine,
     MemoryRegion *ret = host_memory_backend_get_memory(backend);
 
     if (memory_region_is_mapped(ret)) {
-        char *path = object_get_canonical_path_component(OBJECT(backend));
-        error_report("memory backend %s can't be used multiple times.", path);
-        g_free(path);
+        error_report("memory backend %s can't be used multiple times.",
+                     object_get_canonical_path_component(OBJECT(backend)));
         exit(EXIT_FAILURE);
     }
     host_memory_backend_set_mapped(backend, true);
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index d0d6e553cf..e1574bc07c 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -137,13 +137,12 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
 
     if (size <= nvdimm->label_size || !pmem_size) {
         HostMemoryBackend *hostmem = dimm->hostmem;
-        char *path = object_get_canonical_path_component(OBJECT(hostmem));
 
         error_setg(errp, "the size of memdev %s (0x%" PRIx64 ") is too "
                    "small to contain nvdimm label (0x%" PRIx64 ") and "
                    "aligned PMEM (0x%" PRIx64 ")",
-                   path, memory_region_size(mr), nvdimm->label_size, align);
-        g_free(path);
+                   object_get_canonical_path_component(OBJECT(hostmem)),
+                   memory_region_size(mr), nvdimm->label_size, align);
         return;
     }
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9d3f0b9691..c30351070b 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -179,9 +179,8 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
         return;
     } else if (host_memory_backend_is_mapped(dimm->hostmem)) {
-        char *path = object_get_canonical_path_component(OBJECT(dimm->hostmem));
-        error_setg(errp, "can't use already busy memdev: %s", path);
-        g_free(path);
+        error_setg(errp, "can't use already busy memdev: %s",
+                   object_get_canonical_path_component(OBJECT(dimm->hostmem)));
         return;
     }
     if (((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) ||
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index fc128b25e2..2b6882face 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -1037,9 +1037,8 @@ static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
         error_setg(errp, "You must specify a 'memdev'");
         return;
     } else if (host_memory_backend_is_mapped(s->hostmem)) {
-        char *path = object_get_canonical_path_component(OBJECT(s->hostmem));
-        error_setg(errp, "can't use already busy memdev: %s", path);
-        g_free(path);
+        error_setg(errp, "can't use already busy memdev: %s",
+                   object_get_canonical_path_component(OBJECT(s->hostmem)));
         return;
     }
 
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 43d12bc33a..fe998d8108 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -513,7 +513,7 @@ static void realize(DeviceState *d, Error **errp)
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
     Object *root_container;
     gchar *link_name;
-    char *child_name;
+    const char *child_name;
 
     trace_spapr_drc_realize(spapr_drc_index(drc));
     /* NOTE: we do this as part of realize/unrealize due to the fact
@@ -529,7 +529,6 @@ static void realize(DeviceState *d, Error **errp)
     trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
     object_property_add_alias(root_container, link_name,
                               drc->owner, child_name);
-    g_free(child_name);
     g_free(link_name);
     vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
                      drc);
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index bd9165c565..6da12e315f 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -786,9 +786,8 @@ static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "'cryptodev' parameter expects a valid object");
         return;
     } else if (cryptodev_backend_is_used(vcrypto->cryptodev)) {
-        char *path = object_get_canonical_path_component(OBJECT(vcrypto->conf.cryptodev));
-        error_setg(errp, "can't use already used cryptodev backend: %s", path);
-        g_free(path);
+        error_setg(errp, "can't use already used cryptodev backend: %s",
+                   object_get_canonical_path_component(OBJECT(vcrypto->conf.cryptodev)));
         return;
     }
 
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 65850530e7..c12e9f79b0 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -409,11 +409,9 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "'%s' property is not set", VIRTIO_MEM_MEMDEV_PROP);
         return;
     } else if (host_memory_backend_is_mapped(vmem->memdev)) {
-        char *path = object_get_canonical_path_component(OBJECT(vmem->memdev));
-
         error_setg(errp, "'%s' property specifies a busy memdev: %s",
-                   VIRTIO_MEM_MEMDEV_PROP, path);
-        g_free(path);
+                   VIRTIO_MEM_MEMDEV_PROP,
+                   object_get_canonical_path_component(OBJECT(vmem->memdev)));
         return;
     } else if (!memory_region_is_ram(&vmem->memdev->mr) ||
         memory_region_is_rom(&vmem->memdev->mr) ||
diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index c3374b2f3f..1e0c137497 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -112,9 +112,8 @@ static void virtio_pmem_realize(DeviceState *dev, Error **errp)
     }
 
     if (host_memory_backend_is_mapped(pmem->memdev)) {
-        char *path = object_get_canonical_path_component(OBJECT(pmem->memdev));
-        error_setg(errp, "can't use already busy memdev: %s", path);
-        g_free(path);
+        error_setg(errp, "can't use already busy memdev: %s",
+                   object_get_canonical_path_component(OBJECT(pmem->memdev)));
         return;
     }
 
diff --git a/iothread.c b/iothread.c
index 0598a6d20d..263ec6e5bc 100644
--- a/iothread.c
+++ b/iothread.c
@@ -165,7 +165,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
 {
     Error *local_error = NULL;
     IOThread *iothread = IOTHREAD(obj);
-    char *name, *thread_name;
+    char *thread_name;
 
     iothread->stopping = false;
     iothread->running = true;
@@ -195,12 +195,11 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
     /* This assumes we are called from a thread with useful CPU affinity for us
      * to inherit.
      */
-    name = object_get_canonical_path_component(OBJECT(obj));
-    thread_name = g_strdup_printf("IO %s", name);
+    thread_name = g_strdup_printf("IO %s",
+                        object_get_canonical_path_component(OBJECT(obj)));
     qemu_thread_create(&iothread->thread, thread_name, iothread_run,
                        iothread, QEMU_THREAD_JOINABLE);
     g_free(thread_name);
-    g_free(name);
 
     /* Wait for initialization to complete */
     while (iothread->thread_id == -1) {
@@ -303,7 +302,7 @@ type_init(iothread_register_types)
 
 char *iothread_get_id(IOThread *iothread)
 {
-    return object_get_canonical_path_component(OBJECT(iothread));
+    return g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
 }
 
 AioContext *iothread_get_aio_context(IOThread *iothread)
diff --git a/net/net.c b/net/net.c
index 7fddcebaa2..bbaedb3c7a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1185,12 +1185,10 @@ void print_net_client(Monitor *mon, NetClientState *nc)
         monitor_printf(mon, "filters:\n");
     }
     QTAILQ_FOREACH(nf, &nc->filters, next) {
-        char *path = object_get_canonical_path_component(OBJECT(nf));
-
-        monitor_printf(mon, "  - %s: type=%s", path,
+        monitor_printf(mon, "  - %s: type=%s",
+                       object_get_canonical_path_component(OBJECT(nf)),
                        object_get_typename(OBJECT(nf)));
         netfilter_print_info(mon, nf);
-        g_free(path);
     }
 }
 
diff --git a/qom/object.c b/qom/object.c
index 76f5f75239..00fdf89b3b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1931,7 +1931,7 @@ object_property_add_const_link(Object *obj, const char *name,
                                 NULL, OBJ_PROP_LINK_DIRECT);
 }
 
-char *object_get_canonical_path_component(const Object *obj)
+const char *object_get_canonical_path_component(const Object *obj)
 {
     ObjectProperty *prop = NULL;
     GHashTableIter iter;
@@ -1947,7 +1947,7 @@ char *object_get_canonical_path_component(const Object *obj)
         }
 
         if (prop->opaque == obj) {
-            return g_strdup(prop->name);
+            return prop->name;
         }
     }
 
@@ -1966,7 +1966,7 @@ char *object_get_canonical_path(const Object *obj)
     }
 
     do {
-        char *component = object_get_canonical_path_component(obj);
+        const char *component = object_get_canonical_path_component(obj);
 
         if (!component) {
             /* A canonical path must be complete, so discard what was
@@ -1978,7 +1978,6 @@ char *object_get_canonical_path(const Object *obj)
 
         newpath = g_strdup_printf("/%s%s", component, path ? path : "");
         g_free(path);
-        g_free(component);
         path = newpath;
         obj = obj->parent;
     } while (obj != root);
diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index aaacadacca..4032c96089 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -96,10 +96,8 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
 
 static int qom_composition_compare(const void *a, const void *b, void *ignore)
 {
-    g_autofree char *ac = object_get_canonical_path_component(a);
-    g_autofree char *bc = object_get_canonical_path_component(b);
-
-    return g_strcmp0(ac, bc);
+    return g_strcmp0(object_get_canonical_path_component(a),
+                     object_get_canonical_path_component(b));
 }
 
 static int insert_qom_composition_child(Object *obj, void *opaque)
@@ -112,18 +110,17 @@ static int insert_qom_composition_child(Object *obj, void *opaque)
 
 static void print_qom_composition(Monitor *mon, Object *obj, int indent)
 {
-    char *name;
+    const char *name;
     GQueue children;
     Object *child;
 
     if (obj == object_get_root()) {
-        name = g_strdup("");
+        name = "";
     } else {
         name = object_get_canonical_path_component(obj);
     }
     monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
                    object_get_typename(obj));
-    g_free(name);
 
     g_queue_init(&children);
     object_child_foreach(obj, insert_qom_composition_child, &children);
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index bf62cbec11..5acccfb4e3 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -42,11 +42,10 @@ typedef struct PRManagerHelper {
 
 static void pr_manager_send_status_changed_event(PRManagerHelper *pr_mgr)
 {
-    char *id = object_get_canonical_path_component(OBJECT(pr_mgr));
+    const char *id = object_get_canonical_path_component(OBJECT(pr_mgr));
 
     if (id) {
         qapi_event_send_pr_manager_status_changed(id, !!pr_mgr->ioc);
-        g_free(id);
     }
 }
 
diff --git a/scsi/pr-manager.c b/scsi/pr-manager.c
index 0c866e8698..32b9287e68 100644
--- a/scsi/pr-manager.c
+++ b/scsi/pr-manager.c
@@ -128,7 +128,7 @@ static int query_one_pr_manager(Object *object, void *opaque)
 
     elem = g_new0(PRManagerInfoList, 1);
     info = g_new0(PRManagerInfo, 1);
-    info->id = object_get_canonical_path_component(object);
+    info->id = g_strdup(object_get_canonical_path_component(object));
     info->connected = pr_manager_is_connected(pr_mgr);
     elem->value = info;
     elem->next = NULL;
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 9200b20130..af25987518 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1764,7 +1764,7 @@ const char *memory_region_name(const MemoryRegion *mr)
 {
     if (!mr->name) {
         ((MemoryRegion *)mr)->name =
-            object_get_canonical_path_component(OBJECT(mr));
+            g_strdup(object_get_canonical_path_component(OBJECT(mr)));
     }
     return mr->name;
 }
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 9ea620f23c..7c0be4102e 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -57,7 +57,7 @@ spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
 spapr_drc_awaiting_quiesce(uint32_t index) "drc: 0x%"PRIx32
 spapr_drc_reset(uint32_t index) "drc: 0x%"PRIx32
 spapr_drc_realize(uint32_t index) "drc: 0x%"PRIx32
-spapr_drc_realize_child(uint32_t index, char *childname) "drc: 0x%"PRIx32", child name: %s"
+spapr_drc_realize_child(uint32_t index, const char *childname) "drc: 0x%"PRIx32", child name: %s"
 spapr_drc_realize_complete(uint32_t index) "drc: 0x%"PRIx32
 spapr_drc_unrealize(uint32_t index) "drc: 0x%"PRIx32
 
-- 
2.26.2

Re: [PATCH for-5.1 3/5] qom: Change object_get_canonical_path_component() not to malloc

[Philippe Mathieu-Daudé]

On 7/14/20 6:02 PM, Markus Armbruster wrote:
> object_get_canonical_path_component() returns a malloced copy of a
> property name on success, null on failure.
> 
> 19 of its 25 callers immediately free the returned copy.
> 
> Change object_get_canonical_path_component() to return the property
> name directly.  Since modifying the name would be wrong, adjust the
> return type to const char *.

Yeah!

> 
> Drop the free from the 19 callers become simpler, add the g_strdup()
> to the other six.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qom/object.h       |  2 +-
>  backends/hostmem.c         |  2 +-
>  block/throttle-groups.c    |  2 +-
>  gdbstub.c                  |  2 +-
>  hw/arm/xlnx-zynqmp.c       |  6 ++----
>  hw/block/nvme.c            |  5 ++---
>  hw/core/machine-qmp-cmds.c |  2 +-
>  hw/core/machine.c          |  5 ++---
>  hw/mem/nvdimm.c            |  5 ++---
>  hw/mem/pc-dimm.c           |  5 ++---
>  hw/misc/ivshmem.c          |  5 ++---
>  hw/ppc/spapr_drc.c         |  3 +--
>  hw/virtio/virtio-crypto.c  |  5 ++---
>  hw/virtio/virtio-mem.c     |  6 ++----
>  hw/virtio/virtio-pmem.c    |  5 ++---
>  iothread.c                 |  9 ++++-----
>  net/net.c                  |  6 ++----
>  qom/object.c               |  7 +++----
>  qom/qom-hmp-cmds.c         | 11 ++++-------
>  scsi/pr-manager-helper.c   |  3 +--
>  scsi/pr-manager.c          |  2 +-
>  softmmu/memory.c           |  2 +-
>  hw/ppc/trace-events        |  2 +-
>  23 files changed, 41 insertions(+), 61 deletions(-)
...

> @@ -303,7 +302,7 @@ type_init(iothread_register_types)
>  
>  char *iothread_get_id(IOThread *iothread)
>  {
> -    return object_get_canonical_path_component(OBJECT(iothread));
> +    return g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
>  }

(Note for later, this one can return const).

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-5.1 3/5] qom: Change object_get_canonical_path_component() not to malloc

[Li Qiang]

Markus Armbruster <armbru@redhat.com> 于2020年7月15日周三 上午12:08写道：
>
> object_get_canonical_path_component() returns a malloced copy of a
> property name on success, null on failure.
>
> 19 of its 25 callers immediately free the returned copy.
>
> Change object_get_canonical_path_component() to return the property
> name directly.  Since modifying the name would be wrong, adjust the
> return type to const char *.
>
> Drop the free from the 19 callers become simpler, add the g_strdup()
> to the other six.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Yes, I have seen more than once memory leak caused by this function.

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  include/qom/object.h       |  2 +-
>  backends/hostmem.c         |  2 +-
>  block/throttle-groups.c    |  2 +-
>  gdbstub.c                  |  2 +-
>  hw/arm/xlnx-zynqmp.c       |  6 ++----
>  hw/block/nvme.c            |  5 ++---
>  hw/core/machine-qmp-cmds.c |  2 +-
>  hw/core/machine.c          |  5 ++---
>  hw/mem/nvdimm.c            |  5 ++---
>  hw/mem/pc-dimm.c           |  5 ++---
>  hw/misc/ivshmem.c          |  5 ++---
>  hw/ppc/spapr_drc.c         |  3 +--
>  hw/virtio/virtio-crypto.c  |  5 ++---
>  hw/virtio/virtio-mem.c     |  6 ++----
>  hw/virtio/virtio-pmem.c    |  5 ++---
>  iothread.c                 |  9 ++++-----
>  net/net.c                  |  6 ++----
>  qom/object.c               |  7 +++----
>  qom/qom-hmp-cmds.c         | 11 ++++-------
>  scsi/pr-manager-helper.c   |  3 +--
>  scsi/pr-manager.c          |  2 +-
>  softmmu/memory.c           |  2 +-
>  hw/ppc/trace-events        |  2 +-
>  23 files changed, 41 insertions(+), 61 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 79c8f838b6..55d925d2c8 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1462,7 +1462,7 @@ Object *object_get_internal_root(void);
>   * path is the path within the composition tree starting from the root.
>   * %NULL if the object doesn't have a parent (and thus a canonical path).
>   */
> -char *object_get_canonical_path_component(const Object *obj);
> +const char *object_get_canonical_path_component(const Object *obj);
>
>  /**
>   * object_get_canonical_path:
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index c614f1bdc1..4bde00e8e7 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -33,7 +33,7 @@ char *
>  host_memory_backend_get_name(HostMemoryBackend *backend)
>  {
>      if (!backend->use_canonical_path) {
> -        return object_get_canonical_path_component(OBJECT(backend));
> +        return g_strdup(object_get_canonical_path_component(OBJECT(backend)));
>      }
>
>      return object_get_canonical_path(OBJECT(backend));
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 03a53c89ea..98fea7fd47 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -771,7 +771,7 @@ static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
>
>      /* set group name to object id if it exists */
>      if (!tg->name && tg->parent_obj.parent) {
> -        tg->name = object_get_canonical_path_component(OBJECT(obj));
> +        tg->name = g_strdup(object_get_canonical_path_component(OBJECT(obj)));
>      }
>      /* We must have a group name at this point */
>      assert(tg->name);
> diff --git a/gdbstub.c b/gdbstub.c
> index 6950fd243f..f3a318cd7f 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2059,7 +2059,7 @@ static void handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
>          /* Print the CPU model and name in multiprocess mode */
>          ObjectClass *oc = object_get_class(OBJECT(cpu));
>          const char *cpu_model = object_class_get_name(oc);
> -        g_autofree char *cpu_name =
> +        const char *cpu_name =
>              object_get_canonical_path_component(OBJECT(cpu));
>          g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
>                          cpu->halted ? "halted " : "running");
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 772cfa3771..e14323c991 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -190,7 +190,7 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
>      qdev_prop_set_uint32(DEVICE(&s->rpu_cluster), "cluster-id", 1);
>
>      for (i = 0; i < num_rpus; i++) {
> -        char *name;
> +        const char *name;
>
>          object_initialize_child(OBJECT(&s->rpu_cluster), "rpu-cpu[*]",
>                                  &s->rpu_cpu[i],
> @@ -204,7 +204,6 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
>          } else {
>              s->boot_cpu_ptr = &s->rpu_cpu[i];
>          }
> -        g_free(name);
>
>          object_property_set_bool(OBJECT(&s->rpu_cpu[i]), "reset-hivecs", true,
>                                   &error_abort);
> @@ -341,7 +340,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>
>      /* Realize APUs before realizing the GIC. KVM requires this.  */
>      for (i = 0; i < num_apus; i++) {
> -        char *name;
> +        const char *name;
>
>          object_property_set_int(OBJECT(&s->apu_cpu[i]), "psci-conduit",
>                                  QEMU_PSCI_CONDUIT_SMC, &error_abort);
> @@ -354,7 +353,6 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          } else {
>              s->boot_cpu_ptr = &s->apu_cpu[i];
>          }
> -        g_free(name);
>
>          object_property_set_bool(OBJECT(&s->apu_cpu[i]), "has_el3", s->secure,
>                                   NULL);
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 1aee042d4c..3426e17e65 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1397,9 +1397,8 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
>
>      if (!n->params.cmb_size_mb && n->pmrdev) {
>          if (host_memory_backend_is_mapped(n->pmrdev)) {
> -            char *path = object_get_canonical_path_component(OBJECT(n->pmrdev));
> -            error_setg(errp, "can't use already busy memdev: %s", path);
> -            g_free(path);
> +            error_setg(errp, "can't use already busy memdev: %s",
> +                       object_get_canonical_path_component(OBJECT(n->pmrdev)));
>              return;
>          }
>
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 2c5da8413d..963088b798 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -315,7 +315,7 @@ static int query_memdev(Object *obj, void *opaque)
>
>          m->value = g_malloc0(sizeof(*m->value));
>
> -        m->value->id = object_get_canonical_path_component(obj);
> +        m->value->id = g_strdup(object_get_canonical_path_component(obj));
>          m->value->has_id = !!m->value->id;
>
>          m->value->size = object_property_get_uint(obj, "size",
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index eb267b828d..2f881d6d75 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1073,9 +1073,8 @@ MemoryRegion *machine_consume_memdev(MachineState *machine,
>      MemoryRegion *ret = host_memory_backend_get_memory(backend);
>
>      if (memory_region_is_mapped(ret)) {
> -        char *path = object_get_canonical_path_component(OBJECT(backend));
> -        error_report("memory backend %s can't be used multiple times.", path);
> -        g_free(path);
> +        error_report("memory backend %s can't be used multiple times.",
> +                     object_get_canonical_path_component(OBJECT(backend)));
>          exit(EXIT_FAILURE);
>      }
>      host_memory_backend_set_mapped(backend, true);
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index d0d6e553cf..e1574bc07c 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -137,13 +137,12 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
>
>      if (size <= nvdimm->label_size || !pmem_size) {
>          HostMemoryBackend *hostmem = dimm->hostmem;
> -        char *path = object_get_canonical_path_component(OBJECT(hostmem));
>
>          error_setg(errp, "the size of memdev %s (0x%" PRIx64 ") is too "
>                     "small to contain nvdimm label (0x%" PRIx64 ") and "
>                     "aligned PMEM (0x%" PRIx64 ")",
> -                   path, memory_region_size(mr), nvdimm->label_size, align);
> -        g_free(path);
> +                   object_get_canonical_path_component(OBJECT(hostmem)),
> +                   memory_region_size(mr), nvdimm->label_size, align);
>          return;
>      }
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 9d3f0b9691..c30351070b 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -179,9 +179,8 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
>          error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
>          return;
>      } else if (host_memory_backend_is_mapped(dimm->hostmem)) {
> -        char *path = object_get_canonical_path_component(OBJECT(dimm->hostmem));
> -        error_setg(errp, "can't use already busy memdev: %s", path);
> -        g_free(path);
> +        error_setg(errp, "can't use already busy memdev: %s",
> +                   object_get_canonical_path_component(OBJECT(dimm->hostmem)));
>          return;
>      }
>      if (((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) ||
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index fc128b25e2..2b6882face 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1037,9 +1037,8 @@ static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
>          error_setg(errp, "You must specify a 'memdev'");
>          return;
>      } else if (host_memory_backend_is_mapped(s->hostmem)) {
> -        char *path = object_get_canonical_path_component(OBJECT(s->hostmem));
> -        error_setg(errp, "can't use already busy memdev: %s", path);
> -        g_free(path);
> +        error_setg(errp, "can't use already busy memdev: %s",
> +                   object_get_canonical_path_component(OBJECT(s->hostmem)));
>          return;
>      }
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 43d12bc33a..fe998d8108 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -513,7 +513,7 @@ static void realize(DeviceState *d, Error **errp)
>      SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
>      Object *root_container;
>      gchar *link_name;
> -    char *child_name;
> +    const char *child_name;
>
>      trace_spapr_drc_realize(spapr_drc_index(drc));
>      /* NOTE: we do this as part of realize/unrealize due to the fact
> @@ -529,7 +529,6 @@ static void realize(DeviceState *d, Error **errp)
>      trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
>      object_property_add_alias(root_container, link_name,
>                                drc->owner, child_name);
> -    g_free(child_name);
>      g_free(link_name);
>      vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
>                       drc);
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index bd9165c565..6da12e315f 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -786,9 +786,8 @@ static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
>          error_setg(errp, "'cryptodev' parameter expects a valid object");
>          return;
>      } else if (cryptodev_backend_is_used(vcrypto->cryptodev)) {
> -        char *path = object_get_canonical_path_component(OBJECT(vcrypto->conf.cryptodev));
> -        error_setg(errp, "can't use already used cryptodev backend: %s", path);
> -        g_free(path);
> +        error_setg(errp, "can't use already used cryptodev backend: %s",
> +                   object_get_canonical_path_component(OBJECT(vcrypto->conf.cryptodev)));
>          return;
>      }
>
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 65850530e7..c12e9f79b0 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -409,11 +409,9 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
>          error_setg(errp, "'%s' property is not set", VIRTIO_MEM_MEMDEV_PROP);
>          return;
>      } else if (host_memory_backend_is_mapped(vmem->memdev)) {
> -        char *path = object_get_canonical_path_component(OBJECT(vmem->memdev));
> -
>          error_setg(errp, "'%s' property specifies a busy memdev: %s",
> -                   VIRTIO_MEM_MEMDEV_PROP, path);
> -        g_free(path);
> +                   VIRTIO_MEM_MEMDEV_PROP,
> +                   object_get_canonical_path_component(OBJECT(vmem->memdev)));
>          return;
>      } else if (!memory_region_is_ram(&vmem->memdev->mr) ||
>          memory_region_is_rom(&vmem->memdev->mr) ||
> diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
> index c3374b2f3f..1e0c137497 100644
> --- a/hw/virtio/virtio-pmem.c
> +++ b/hw/virtio/virtio-pmem.c
> @@ -112,9 +112,8 @@ static void virtio_pmem_realize(DeviceState *dev, Error **errp)
>      }
>
>      if (host_memory_backend_is_mapped(pmem->memdev)) {
> -        char *path = object_get_canonical_path_component(OBJECT(pmem->memdev));
> -        error_setg(errp, "can't use already busy memdev: %s", path);
> -        g_free(path);
> +        error_setg(errp, "can't use already busy memdev: %s",
> +                   object_get_canonical_path_component(OBJECT(pmem->memdev)));
>          return;
>      }
>
> diff --git a/iothread.c b/iothread.c
> index 0598a6d20d..263ec6e5bc 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -165,7 +165,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
>  {
>      Error *local_error = NULL;
>      IOThread *iothread = IOTHREAD(obj);
> -    char *name, *thread_name;
> +    char *thread_name;
>
>      iothread->stopping = false;
>      iothread->running = true;
> @@ -195,12 +195,11 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
>      /* This assumes we are called from a thread with useful CPU affinity for us
>       * to inherit.
>       */
> -    name = object_get_canonical_path_component(OBJECT(obj));
> -    thread_name = g_strdup_printf("IO %s", name);
> +    thread_name = g_strdup_printf("IO %s",
> +                        object_get_canonical_path_component(OBJECT(obj)));
>      qemu_thread_create(&iothread->thread, thread_name, iothread_run,
>                         iothread, QEMU_THREAD_JOINABLE);
>      g_free(thread_name);
> -    g_free(name);
>
>      /* Wait for initialization to complete */
>      while (iothread->thread_id == -1) {
> @@ -303,7 +302,7 @@ type_init(iothread_register_types)
>
>  char *iothread_get_id(IOThread *iothread)
>  {
> -    return object_get_canonical_path_component(OBJECT(iothread));
> +    return g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
>  }
>
>  AioContext *iothread_get_aio_context(IOThread *iothread)
> diff --git a/net/net.c b/net/net.c
> index 7fddcebaa2..bbaedb3c7a 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1185,12 +1185,10 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>          monitor_printf(mon, "filters:\n");
>      }
>      QTAILQ_FOREACH(nf, &nc->filters, next) {
> -        char *path = object_get_canonical_path_component(OBJECT(nf));
> -
> -        monitor_printf(mon, "  - %s: type=%s", path,
> +        monitor_printf(mon, "  - %s: type=%s",
> +                       object_get_canonical_path_component(OBJECT(nf)),
>                         object_get_typename(OBJECT(nf)));
>          netfilter_print_info(mon, nf);
> -        g_free(path);
>      }
>  }
>
> diff --git a/qom/object.c b/qom/object.c
> index 76f5f75239..00fdf89b3b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1931,7 +1931,7 @@ object_property_add_const_link(Object *obj, const char *name,
>                                  NULL, OBJ_PROP_LINK_DIRECT);
>  }
>
> -char *object_get_canonical_path_component(const Object *obj)
> +const char *object_get_canonical_path_component(const Object *obj)
>  {
>      ObjectProperty *prop = NULL;
>      GHashTableIter iter;
> @@ -1947,7 +1947,7 @@ char *object_get_canonical_path_component(const Object *obj)
>          }
>
>          if (prop->opaque == obj) {
> -            return g_strdup(prop->name);
> +            return prop->name;
>          }
>      }
>
> @@ -1966,7 +1966,7 @@ char *object_get_canonical_path(const Object *obj)
>      }
>
>      do {
> -        char *component = object_get_canonical_path_component(obj);
> +        const char *component = object_get_canonical_path_component(obj);
>
>          if (!component) {
>              /* A canonical path must be complete, so discard what was
> @@ -1978,7 +1978,6 @@ char *object_get_canonical_path(const Object *obj)
>
>          newpath = g_strdup_printf("/%s%s", component, path ? path : "");
>          g_free(path);
> -        g_free(component);
>          path = newpath;
>          obj = obj->parent;
>      } while (obj != root);
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index aaacadacca..4032c96089 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -96,10 +96,8 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>
>  static int qom_composition_compare(const void *a, const void *b, void *ignore)
>  {
> -    g_autofree char *ac = object_get_canonical_path_component(a);
> -    g_autofree char *bc = object_get_canonical_path_component(b);
> -
> -    return g_strcmp0(ac, bc);
> +    return g_strcmp0(object_get_canonical_path_component(a),
> +                     object_get_canonical_path_component(b));
>  }
>
>  static int insert_qom_composition_child(Object *obj, void *opaque)
> @@ -112,18 +110,17 @@ static int insert_qom_composition_child(Object *obj, void *opaque)
>
>  static void print_qom_composition(Monitor *mon, Object *obj, int indent)
>  {
> -    char *name;
> +    const char *name;
>      GQueue children;
>      Object *child;
>
>      if (obj == object_get_root()) {
> -        name = g_strdup("");
> +        name = "";
>      } else {
>          name = object_get_canonical_path_component(obj);
>      }
>      monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
>                     object_get_typename(obj));
> -    g_free(name);
>
>      g_queue_init(&children);
>      object_child_foreach(obj, insert_qom_composition_child, &children);
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> index bf62cbec11..5acccfb4e3 100644
> --- a/scsi/pr-manager-helper.c
> +++ b/scsi/pr-manager-helper.c
> @@ -42,11 +42,10 @@ typedef struct PRManagerHelper {
>
>  static void pr_manager_send_status_changed_event(PRManagerHelper *pr_mgr)
>  {
> -    char *id = object_get_canonical_path_component(OBJECT(pr_mgr));
> +    const char *id = object_get_canonical_path_component(OBJECT(pr_mgr));
>
>      if (id) {
>          qapi_event_send_pr_manager_status_changed(id, !!pr_mgr->ioc);
> -        g_free(id);
>      }
>  }
>
> diff --git a/scsi/pr-manager.c b/scsi/pr-manager.c
> index 0c866e8698..32b9287e68 100644
> --- a/scsi/pr-manager.c
> +++ b/scsi/pr-manager.c
> @@ -128,7 +128,7 @@ static int query_one_pr_manager(Object *object, void *opaque)
>
>      elem = g_new0(PRManagerInfoList, 1);
>      info = g_new0(PRManagerInfo, 1);
> -    info->id = object_get_canonical_path_component(object);
> +    info->id = g_strdup(object_get_canonical_path_component(object));
>      info->connected = pr_manager_is_connected(pr_mgr);
>      elem->value = info;
>      elem->next = NULL;
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 9200b20130..af25987518 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1764,7 +1764,7 @@ const char *memory_region_name(const MemoryRegion *mr)
>  {
>      if (!mr->name) {
>          ((MemoryRegion *)mr)->name =
> -            object_get_canonical_path_component(OBJECT(mr));
> +            g_strdup(object_get_canonical_path_component(OBJECT(mr)));
>      }
>      return mr->name;
>  }
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 9ea620f23c..7c0be4102e 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -57,7 +57,7 @@ spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_awaiting_quiesce(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_reset(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_realize(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_realize_child(uint32_t index, char *childname) "drc: 0x%"PRIx32", child name: %s"
> +spapr_drc_realize_child(uint32_t index, const char *childname) "drc: 0x%"PRIx32", child name: %s"
>  spapr_drc_realize_complete(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_unrealize(uint32_t index) "drc: 0x%"PRIx32
>
> --
> 2.26.2
>
>

[PATCH for-5.1 4/5] qom: Document object_get_canonical_path() returns malloced string

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qom/object.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 55d925d2c8..0f3a60617c 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1467,8 +1467,9 @@ const char *object_get_canonical_path_component(const Object *obj);
 /**
  * object_get_canonical_path:
  *
- * Returns: The canonical path for a object.  This is the path within the
- * composition tree starting from the root.
+ * Returns: The canonical path for a object, newly allocated.  This is
+ * the path within the composition tree starting from the root.  Use
+ * g_free() to free it.
  */
 char *object_get_canonical_path(const Object *obj);
 
-- 
2.26.2

Re: [PATCH for-5.1 4/5] qom: Document object_get_canonical_path() returns malloced string

[Philippe Mathieu-Daudé]

On 7/14/20 6:02 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qom/object.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 55d925d2c8..0f3a60617c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1467,8 +1467,9 @@ const char *object_get_canonical_path_component(const Object *obj);
>  /**
>   * object_get_canonical_path:
>   *
> - * Returns: The canonical path for a object.  This is the path within the
> - * composition tree starting from the root.
> + * Returns: The canonical path for a object, newly allocated.  This is
> + * the path within the composition tree starting from the root.  Use
> + * g_free() to free it.
>   */
>  char *object_get_canonical_path(const Object *obj);
>  

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH for-5.1 5/5] qom: Make info qom-tree sort children more efficiently

[Markus Armbruster]

Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
sorts children the simple, stupid, quadratic way.  I thought the
number of children would be small enough for this not to matter.  I
was wrong: there are outliers with several hundred children, e.g ARM
machines nuri and smdkc210 each have a node with 513 children.

While n^2 sorting isn't noticeable in normal, human usage even for
n=513, it can be quite noticeable in certain automated tests.  In
particular, the sort made device-introspect-test even slower.  Commit
3e7b80f84d "tests: improve performance of device-introspect-test" just
fixed that by cutting back its excessive use of "info qom-tree".
Sorting more efficiently makes sense regardless, so do it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qom/qom-hmp-cmds.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index 4032c96089..8861a109d5 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -94,25 +94,23 @@ typedef struct QOMCompositionState {
 
 static void print_qom_composition(Monitor *mon, Object *obj, int indent);
 
-static int qom_composition_compare(const void *a, const void *b, void *ignore)
+static int qom_composition_compare(const void *a, const void *b)
 {
-    return g_strcmp0(object_get_canonical_path_component(a),
-                     object_get_canonical_path_component(b));
+    return g_strcmp0(object_get_canonical_path_component(*(Object **)a),
+                     object_get_canonical_path_component(*(Object **)b));
 }
 
 static int insert_qom_composition_child(Object *obj, void *opaque)
 {
-    GQueue *children = opaque;
-
-    g_queue_insert_sorted(children, obj, qom_composition_compare, NULL);
+    g_array_append_val(opaque, obj);
     return 0;
 }
 
 static void print_qom_composition(Monitor *mon, Object *obj, int indent)
 {
+    GArray *children = g_array_new(false, false, sizeof(Object *));
     const char *name;
-    GQueue children;
-    Object *child;
+    int i;
 
     if (obj == object_get_root()) {
         name = "";
@@ -122,11 +120,14 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent)
     monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
                    object_get_typename(obj));
 
-    g_queue_init(&children);
-    object_child_foreach(obj, insert_qom_composition_child, &children);
-    while ((child = g_queue_pop_head(&children))) {
-        print_qom_composition(mon, child, indent + 2);
+    object_child_foreach(obj, insert_qom_composition_child, children);
+    g_array_sort(children, qom_composition_compare);
+
+    for (i = 0; i < children->len; i++) {
+        print_qom_composition(mon, g_array_index(children, Object *, i),
+                              indent + 2);
     }
+    g_array_free(children, TRUE);
 }
 
 void hmp_info_qom_tree(Monitor *mon, const QDict *dict)
-- 
2.26.2

Re: [PATCH for-5.1 5/5] qom: Make info qom-tree sort children more efficiently

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> sorts children the simple, stupid, quadratic way.  I thought the
> number of children would be small enough for this not to matter.  I
> was wrong: there are outliers with several hundred children, e.g ARM
> machines nuri and smdkc210 each have a node with 513 children.

Big Power systems can have thousands.

> While n^2 sorting isn't noticeable in normal, human usage even for
> n=513, it can be quite noticeable in certain automated tests.  In
> particular, the sort made device-introspect-test even slower.  Commit
> 3e7b80f84d "tests: improve performance of device-introspect-test" just
> fixed that by cutting back its excessive use of "info qom-tree".
> Sorting more efficiently makes sense regardless, so do it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qom/qom-hmp-cmds.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 4032c96089..8861a109d5 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -94,25 +94,23 @@ typedef struct QOMCompositionState {
>  
>  static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>  
> -static int qom_composition_compare(const void *a, const void *b, void *ignore)
> +static int qom_composition_compare(const void *a, const void *b)
>  {
> -    return g_strcmp0(object_get_canonical_path_component(a),
> -                     object_get_canonical_path_component(b));
> +    return g_strcmp0(object_get_canonical_path_component(*(Object **)a),
> +                     object_get_canonical_path_component(*(Object **)b));
>  }
>  
>  static int insert_qom_composition_child(Object *obj, void *opaque)
>  {
> -    GQueue *children = opaque;
> -
> -    g_queue_insert_sorted(children, obj, qom_composition_compare, NULL);
> +    g_array_append_val(opaque, obj);
>      return 0;
>  }
>  
>  static void print_qom_composition(Monitor *mon, Object *obj, int indent)
>  {
> +    GArray *children = g_array_new(false, false, sizeof(Object *));
>      const char *name;
> -    GQueue children;
> -    Object *child;
> +    int i;
>  
>      if (obj == object_get_root()) {
>          name = "";
> @@ -122,11 +120,14 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent)
>      monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
>                     object_get_typename(obj));
>  
> -    g_queue_init(&children);
> -    object_child_foreach(obj, insert_qom_composition_child, &children);
> -    while ((child = g_queue_pop_head(&children))) {
> -        print_qom_composition(mon, child, indent + 2);
> +    object_child_foreach(obj, insert_qom_composition_child, children);
> +    g_array_sort(children, qom_composition_compare);
> +
> +    for (i = 0; i < children->len; i++) {
> +        print_qom_composition(mon, g_array_index(children, Object *, i),
> +                              indent + 2);
>      }
> +    g_array_free(children, TRUE);

So I think that's OK, so :

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Can you just convince me that 'TRUE' in the array_free?

Dave

>  }
>  
>  void hmp_info_qom_tree(Monitor *mon, const QDict *dict)
> -- 
> 2.26.2
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH for-5.1 5/5] qom: Make info qom-tree sort children more efficiently

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
>> sorts children the simple, stupid, quadratic way.  I thought the
>> number of children would be small enough for this not to matter.  I
>> was wrong: there are outliers with several hundred children, e.g ARM
>> machines nuri and smdkc210 each have a node with 513 children.
>
> Big Power systems can have thousands.
>
>> While n^2 sorting isn't noticeable in normal, human usage even for
>> n=513, it can be quite noticeable in certain automated tests.  In
>> particular, the sort made device-introspect-test even slower.  Commit
>> 3e7b80f84d "tests: improve performance of device-introspect-test" just
>> fixed that by cutting back its excessive use of "info qom-tree".
>> Sorting more efficiently makes sense regardless, so do it.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qom/qom-hmp-cmds.c | 25 +++++++++++++------------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
>> index 4032c96089..8861a109d5 100644
>> --- a/qom/qom-hmp-cmds.c
>> +++ b/qom/qom-hmp-cmds.c
>> @@ -94,25 +94,23 @@ typedef struct QOMCompositionState {
>>  
>>  static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>>  
>> -static int qom_composition_compare(const void *a, const void *b, void *ignore)
>> +static int qom_composition_compare(const void *a, const void *b)
>>  {
>> -    return g_strcmp0(object_get_canonical_path_component(a),
>> -                     object_get_canonical_path_component(b));
>> +    return g_strcmp0(object_get_canonical_path_component(*(Object **)a),
>> +                     object_get_canonical_path_component(*(Object **)b));
>>  }
>>  
>>  static int insert_qom_composition_child(Object *obj, void *opaque)
>>  {
>> -    GQueue *children = opaque;
>> -
>> -    g_queue_insert_sorted(children, obj, qom_composition_compare, NULL);
>> +    g_array_append_val(opaque, obj);
>>      return 0;
>>  }
>>  
>>  static void print_qom_composition(Monitor *mon, Object *obj, int indent)
>>  {
>> +    GArray *children = g_array_new(false, false, sizeof(Object *));
>>      const char *name;
>> -    GQueue children;
>> -    Object *child;
>> +    int i;
>>  
>>      if (obj == object_get_root()) {
>>          name = "";
>> @@ -122,11 +120,14 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent)
>>      monitor_printf(mon, "%*s/%s (%s)\n", indent, "", name,
>>                     object_get_typename(obj));
>>  
>> -    g_queue_init(&children);
>> -    object_child_foreach(obj, insert_qom_composition_child, &children);
>> -    while ((child = g_queue_pop_head(&children))) {
>> -        print_qom_composition(mon, child, indent + 2);
>> +    object_child_foreach(obj, insert_qom_composition_child, children);
>> +    g_array_sort(children, qom_composition_compare);
>> +
>> +    for (i = 0; i < children->len; i++) {
>> +        print_qom_composition(mon, g_array_index(children, Object *, i),
>> +                              indent + 2);
>>      }
>> +    g_array_free(children, TRUE);
>
> So I think that's OK, so :
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Can you just convince me that 'TRUE' in the array_free?

g_array_free(children, TRUE) frees both children and children->data.  It
returns null.  This is what we want here.

g_array_free(children, FALSE) frees only children, and returns
children->data.  Occasionally useful.

https://developer.gnome.org/glib/2.62/glib-Arrays.html#g-array-free

I definitely would have made this two separate functions.

Thanks!