[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.1 3/5] qom: Change object_get_canonical_path_component(
From: |
Li Qiang |
Subject: |
Re: [PATCH for-5.1 3/5] qom: Change object_get_canonical_path_component() not to malloc |
Date: |
Wed, 15 Jul 2020 23:26:02 +0800 |
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 5/5] qom: Make info qom-tree sort children more efficiently, (continued)