[GEDI] [PATCH v2 7/7] qapi: More complex uses of QAPI_LIST_APPEND
Markus Armbruster
armbru at redhat.com
Thu Nov 19 08:50:19 UTC 2020
Eric Blake <eblake at redhat.com> writes:
> These cases require a bit more thought to review; in each case, the
> code was appending to a list, but not with a FOOList **tail variable.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> block/gluster.c | 13 +---
> block/qapi.c | 14 +----
> dump/dump.c | 22 ++-----
> hw/core/machine-qmp-cmds.c | 125 +++++++++++++++----------------------
> hw/mem/memory-device.c | 12 +---
> hw/pci/pci.c | 60 ++++++------------
> migration/migration.c | 20 ++----
> monitor/hmp-cmds.c | 23 +++----
> net/net.c | 13 +---
> qga/commands-posix.c | 101 +++++++++++-------------------
> qga/commands-win32.c | 88 +++++++++-----------------
> softmmu/tpm.c | 38 ++---------
> ui/spice-core.c | 27 +++-----
> 13 files changed, 180 insertions(+), 376 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 1f8699b93835..4963642d6e6b 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -514,7 +514,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> {
> QemuOpts *opts;
> SocketAddress *gsconf = NULL;
> - SocketAddressList *curr = NULL;
> + SocketAddressList **curr;
Looks like "a FOOList **tail variable" to me. Hmm, unlike the ones in
PATCH 6, its initializer is garbage, and ...
> QDict *backing_options = NULL;
> Error *local_err = NULL;
> char *str = NULL;
> @@ -547,6 +547,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> }
> gconf->path = g_strdup(ptr);
> qemu_opts_del(opts);
> + curr = &gconf->server;
>
> for (i = 0; i < num_servers; i++) {
> str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
> @@ -655,15 +656,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
> qemu_opts_del(opts);
> }
>
> - if (gconf->server == NULL) {
> - gconf->server = g_new0(SocketAddressList, 1);
> - gconf->server->value = gsconf;
> - curr = gconf->server;
> - } else {
> - curr->next = g_new0(SocketAddressList, 1);
> - curr->next->value = gsconf;
> - curr = curr->next;
> - }
> + QAPI_LIST_APPEND(curr, gsconf);
... it is used only from the second list element on. Okay, I see why
this is not in PATCH 6.
> gsconf = NULL;
>
> qobject_unref(backing_options);
[...]
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index ca39d15d93a2..711814be2312 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
[...]
> @@ -294,41 +281,31 @@ void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
> static int query_memdev(Object *obj, void *opaque)
> {
> MemdevList **list = opaque;
> - MemdevList *m = NULL;
> + Memdev *m;
> QObject *host_nodes;
> Visitor *v;
>
> if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
> m = g_malloc0(sizeof(*m));
>
> - m->value = g_malloc0(sizeof(*m->value));
> + m->id = g_strdup(object_get_canonical_path_component(obj));
> + m->has_id = !!m->id;
>
> - 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",
> - &error_abort);
> - m->value->merge = object_property_get_bool(obj, "merge",
> - &error_abort);
> - m->value->dump = object_property_get_bool(obj, "dump",
> - &error_abort);
> - m->value->prealloc = object_property_get_bool(obj,
> - "prealloc",
> - &error_abort);
> - m->value->policy = object_property_get_enum(obj,
> - "policy",
> - "HostMemPolicy",
> - &error_abort);
> + m->size = object_property_get_uint(obj, "size", &error_abort);
> + m->merge = object_property_get_bool(obj, "merge", &error_abort);
> + m->dump = object_property_get_bool(obj, "dump", &error_abort);
> + m->prealloc = object_property_get_bool(obj, "prealloc", &error_abort);
> + m->policy = object_property_get_enum(obj, "policy", "HostMemPolicy",
> + &error_abort);
> host_nodes = object_property_get_qobject(obj,
> "host-nodes",
> &error_abort);
> v = qobject_input_visitor_new(host_nodes);
> - visit_type_uint16List(v, NULL, &m->value->host_nodes, &error_abort);
> + visit_type_uint16List(v, NULL, &m->host_nodes, &error_abort);
> visit_free(v);
> qobject_unref(host_nodes);
>
> - m->next = *list;
> - *list = m;
> + QAPI_LIST_APPEND(list, m);
The old code prepends, doesn't it?
> }
>
> return 0;
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index cf0627fd01c1..1afcc29a0649 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -199,7 +199,7 @@ out:
> MemoryDeviceInfoList *qmp_memory_device_list(void)
> {
> GSList *devices = NULL, *item;
> - MemoryDeviceInfoList *list = NULL, *prev = NULL;
> + MemoryDeviceInfoList *list = NULL, **prev = &list;
Here, you reuse the old name for the new variable.
>
> object_child_foreach(qdev_get_machine(), memory_device_build_list,
> &devices);
> @@ -207,19 +207,11 @@ MemoryDeviceInfoList *qmp_memory_device_list(void)
> for (item = devices; item; item = g_slist_next(item)) {
> const MemoryDeviceState *md = MEMORY_DEVICE(item->data);
> const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(item->data);
> - MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
> MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
>
> mdc->fill_device_info(md, info);
>
> - elem->value = info;
> - elem->next = NULL;
> - if (prev) {
> - prev->next = elem;
> - } else {
> - list = elem;
> - }
> - prev = elem;
> + QAPI_LIST_APPEND(prev, info);
> }
>
> g_slist_free(devices);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 0131d9d02c16..43f19e4ab219 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1681,41 +1681,34 @@ static PciDeviceInfoList *qmp_query_pci_devices(PCIBus *bus, int bus_num);
>
> static PciMemoryRegionList *qmp_query_pci_regions(const PCIDevice *dev)
> {
> - PciMemoryRegionList *head = NULL, *cur_item = NULL;
> + PciMemoryRegionList *head = NULL, **tail = &head;
Here, you use a new and better name.
I'd like to encourage you to name tail pointer variables @tail
elsewhere, too.
> int i;
>
> for (i = 0; i < PCI_NUM_REGIONS; i++) {
> const PCIIORegion *r = &dev->io_regions[i];
> - PciMemoryRegionList *region;
> + PciMemoryRegion *region;
>
> if (!r->size) {
> continue;
> }
>
> region = g_malloc0(sizeof(*region));
> - region->value = g_malloc0(sizeof(*region->value));
>
> if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> - region->value->type = g_strdup("io");
> + region->type = g_strdup("io");
> } else {
> - region->value->type = g_strdup("memory");
> - region->value->has_prefetch = true;
> - region->value->prefetch = !!(r->type & PCI_BASE_ADDRESS_MEM_PREFETCH);
> - region->value->has_mem_type_64 = true;
> - region->value->mem_type_64 = !!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64);
> + region->type = g_strdup("memory");
> + region->has_prefetch = true;
> + region->prefetch = !!(r->type & PCI_BASE_ADDRESS_MEM_PREFETCH);
> + region->has_mem_type_64 = true;
> + region->mem_type_64 = !!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64);
> }
>
> - region->value->bar = i;
> - region->value->address = r->addr;
> - region->value->size = r->size;
> + region->bar = i;
> + region->address = r->addr;
> + region->size = r->size;
>
> - /* XXX: waiting for the qapi to support GSList */
> - if (!cur_item) {
> - head = cur_item = region;
> - } else {
> - cur_item->next = region;
> - cur_item = region;
> - }
> + QAPI_LIST_APPEND(tail, region);
> }
>
> return head;
[...]
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index d8bc40ea9f6e..55087e268cda 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2118,17 +2118,17 @@ void qmp_guest_suspend_hybrid(Error **errp)
> guest_suspend(SUSPEND_MODE_HYBRID, errp);
> }
>
> -static GuestNetworkInterfaceList *
> +static GuestNetworkInterface *
> guest_find_interface(GuestNetworkInterfaceList *head,
> const char *name)
> {
> for (; head; head = head->next) {
> if (strcmp(head->value->name, name) == 0) {
> - break;
> + return head->value;
> }
> }
>
> - return head;
> + return NULL;
> }
>
> static int guest_get_network_stats(const char *name,
> @@ -2197,7 +2197,7 @@ static int guest_get_network_stats(const char *name,
> */
> GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> {
> - GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
> + GuestNetworkInterfaceList *head = NULL, **tail = &head;
> struct ifaddrs *ifap, *ifa;
>
> if (getifaddrs(&ifap) < 0) {
> @@ -2206,9 +2206,10 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> }
>
> for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> - GuestNetworkInterfaceList *info;
> - GuestIpAddressList **address_list = NULL, *address_item = NULL;
> - GuestNetworkInterfaceStat *interface_stat = NULL;
> + GuestNetworkInterface *info;
> + GuestIpAddressList **address_tail;
> + GuestIpAddress *address_item = NULL;
> + GuestNetworkInterfaceStat *interface_stat = NULL;
> char addr4[INET_ADDRSTRLEN];
> char addr6[INET6_ADDRSTRLEN];
> int sock;
> @@ -2222,19 +2223,14 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>
> if (!info) {
> info = g_malloc0(sizeof(*info));
> - info->value = g_malloc0(sizeof(*info->value));
> - info->value->name = g_strdup(ifa->ifa_name);
> + info->name = g_strdup(ifa->ifa_name);
>
> - if (!cur_item) {
> - head = cur_item = info;
> - } else {
> - cur_item->next = info;
> - cur_item = info;
> - }
> + QAPI_LIST_APPEND(tail, info);
> }
>
> - if (!info->value->has_hardware_address &&
> - ifa->ifa_flags & SIOCGIFHWADDR) {
> + address_tail = &info->ip_addresses;
> +
> + if (!info->has_hardware_address && ifa->ifa_flags & SIOCGIFHWADDR) {
Unrelated line break removal.
> /* we haven't obtained HW address yet */
> sock = socket(PF_INET, SOCK_STREAM, 0);
> if (sock == -1) {
> @@ -2243,7 +2239,7 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> }
>
> memset(&ifr, 0, sizeof(ifr));
> - pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->value->name);
> + pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->name);
> if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
> error_setg_errno(errp, errno,
> "failed to get MAC address of %s",
> @@ -2255,13 +2251,13 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> close(sock);
> mac_addr = (unsigned char *) &ifr.ifr_hwaddr.sa_data;
>
> - info->value->hardware_address =
> + info->hardware_address =
> g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
> (int) mac_addr[0], (int) mac_addr[1],
> (int) mac_addr[2], (int) mac_addr[3],
> (int) mac_addr[4], (int) mac_addr[5]);
>
> - info->value->has_hardware_address = true;
> + info->has_hardware_address = true;
> }
>
> if (ifa->ifa_addr &&
> @@ -2274,15 +2270,14 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> }
>
> address_item = g_malloc0(sizeof(*address_item));
> - address_item->value = g_malloc0(sizeof(*address_item->value));
> - address_item->value->ip_address = g_strdup(addr4);
> - address_item->value->ip_address_type = GUEST_IP_ADDRESS_TYPE_IPV4;
> + address_item->ip_address = g_strdup(addr4);
> + address_item->ip_address_type = GUEST_IP_ADDRESS_TYPE_IPV4;
>
> if (ifa->ifa_netmask) {
> /* Count the number of set bits in netmask.
> * This is safe as '1' and '0' cannot be shuffled in netmask. */
> p = &((struct sockaddr_in *)ifa->ifa_netmask)->sin_addr;
> - address_item->value->prefix = ctpop32(((uint32_t *) p)[0]);
> + address_item->prefix = ctpop32(((uint32_t *) p)[0]);
> }
> } else if (ifa->ifa_addr &&
> ifa->ifa_addr->sa_family == AF_INET6) {
> @@ -2294,15 +2289,14 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> }
>
> address_item = g_malloc0(sizeof(*address_item));
> - address_item->value = g_malloc0(sizeof(*address_item->value));
> - address_item->value->ip_address = g_strdup(addr6);
> - address_item->value->ip_address_type = GUEST_IP_ADDRESS_TYPE_IPV6;
> + address_item->ip_address = g_strdup(addr6);
> + address_item->ip_address_type = GUEST_IP_ADDRESS_TYPE_IPV6;
>
> if (ifa->ifa_netmask) {
> /* Count the number of set bits in netmask.
> * This is safe as '1' and '0' cannot be shuffled in netmask. */
> p = &((struct sockaddr_in6 *)ifa->ifa_netmask)->sin6_addr;
> - address_item->value->prefix =
> + address_item->prefix =
> ctpop32(((uint32_t *) p)[0]) +
> ctpop32(((uint32_t *) p)[1]) +
> ctpop32(((uint32_t *) p)[2]) +
> @@ -2314,29 +2308,18 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> continue;
> }
>
> - address_list = &info->value->ip_addresses;
> + QAPI_LIST_APPEND(address_tail, address_item);
>
> - while (*address_list && (*address_list)->next) {
> - address_list = &(*address_list)->next;
> - }
> + info->has_ip_addresses = true;
>
> - if (!*address_list) {
> - *address_list = address_item;
> - } else {
> - (*address_list)->next = address_item;
> - }
> -
> - info->value->has_ip_addresses = true;
Before the patch:
address_list = &info->value->ip_addresses;
while (*address_list && (*address_list)->next) {
address_list = &(*address_list)->next;
}
if (!*address_list) {
*address_list = address_item;
} else {
(*address_list)->next = address_item;
}
Note the loop to advance address list to the tail.
Afterwards (info->value has become info):
address_tail = &info->ip_addresses;
[...]
QAPI_LIST_APPEND(address_tail, address_item);
Not the same, I'm afraid: QAPI_LIST_APPEND() blindly overwrites
info->ip_addresses.
> -
> - if (!info->value->has_statistics) {
> + if (!info->has_statistics) {
> interface_stat = g_malloc0(sizeof(*interface_stat));
> - if (guest_get_network_stats(info->value->name,
> - interface_stat) == -1) {
> - info->value->has_statistics = false;
> + if (guest_get_network_stats(info->name, interface_stat) == -1) {
> + info->has_statistics = false;
> g_free(interface_stat);
> } else {
> - info->value->statistics = interface_stat;
> - info->value->has_statistics = true;
> + info->statistics = interface_stat;
> + info->has_statistics = true;
> }
> }
> }
> @@ -2863,7 +2846,6 @@ qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
>
> while (mem_blks != NULL) {
> GuestMemoryBlockResponse *result;
> - GuestMemoryBlockResponseList *entry;
> GuestMemoryBlock *current_mem_blk = mem_blks->value;
>
> result = g_malloc0(sizeof(*result));
> @@ -2872,11 +2854,7 @@ qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
> if (local_err) { /* should never happen */
> goto err;
> }
> - entry = g_malloc0(sizeof *entry);
> - entry->value = result;
> -
> - *link = entry;
> - link = &entry->next;
> + QAPI_LIST_APPEND(link, result);
> mem_blks = mem_blks->next;
> }
>
This one looks like a candidate for PATCH 6.
> @@ -3107,11 +3085,10 @@ static double ga_get_login_time(struct utmpx *user_info)
> GuestUserList *qmp_guest_get_users(Error **errp)
> {
> GHashTable *cache = NULL;
> - GuestUserList *head = NULL, *cur_item = NULL;
> + GuestUserList *head = NULL, **tail = &head;
> struct utmpx *user_info = NULL;
> gpointer value = NULL;
> GuestUser *user = NULL;
> - GuestUserList *item = NULL;
> double login_time = 0;
>
> cache = g_hash_table_new(g_str_hash, g_str_equal);
> @@ -3134,19 +3111,13 @@ GuestUserList *qmp_guest_get_users(Error **errp)
> continue;
> }
>
> - item = g_new0(GuestUserList, 1);
> - item->value = g_new0(GuestUser, 1);
> - item->value->user = g_strdup(user_info->ut_user);
> - item->value->login_time = ga_get_login_time(user_info);
> + user = g_new0(GuestUser, 1);
> + user->user = g_strdup(user_info->ut_user);
> + user->login_time = ga_get_login_time(user_info);
>
> - g_hash_table_insert(cache, item->value->user, item->value);
> + g_hash_table_insert(cache, user->user, user);
>
> - if (!cur_item) {
> - head = cur_item = item;
> - } else {
> - cur_item->next = item;
> - cur_item = item;
> - }
> + QAPI_LIST_APPEND(tail, user);
> }
> endutxent();
> g_hash_table_destroy(cache);
[...]
More information about the integration
mailing list