Re: [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND
Eric Blake 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
> ---
[...]
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index a5058a3bd15e..10ee740eee1b 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2119,17 +2119,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,
> @@ -2198,7 +2198,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 =
> struct ifaddrs *ifap, *ifa;
>
> if (getifaddrs() < 0) {
> @@ -2207,9 +2207,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;
> @@ -2223,19 +2224,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 = >ip_addresses;
> +
> +if (!info->has_hardware_address && ifa->ifa_flags & SIOCGIFHWADDR) {
Unrelated line break removal. I don't mind.
> /* we haven't obtained HW address yet */
> sock = socket(PF_INET, SOCK_STREAM, 0);
> if (sock == -1) {
> @@ -2244,7 +2240,7 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
> }
>
> memset(, 0, sizeof(ifr));
> -pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->value->name);
> +pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->name);
> if (ioctl(sock, SIOCGIFHWADDR, ) == -1) {
> error_setg_errno(errp, errno,
> "failed to get MAC address of %s",
> @@ -2256,13 +2252,13 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
> close(sock);
> mac_addr = (unsigned char *) _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 &&
> @@ -2275,15 +2271,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;
> -
Re: [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND
24.12.2020 01:11, Eric Blake wrote:
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
---
[..]
diff --git a/migration/migration.c b/migration/migration.c
index 805712488e4d..a676405019d1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -784,29 +784,21 @@ void migrate_send_rp_resume_ack(MigrationIncomingState
*mis, uint32_t value)
MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
{
-MigrationCapabilityStatusList *head = NULL;
-MigrationCapabilityStatusList *caps;
+MigrationCapabilityStatusList *head = NULL, **tail =
+MigrationCapabilityStatus *caps;
MigrationState *s = migrate_get_current();
int i;
-caps = NULL; /* silence compiler warning */
for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
#ifndef CONFIG_LIVE_BLOCK_MIGRATION
if (i == MIGRATION_CAPABILITY_BLOCK) {
continue;
}
#endif
-if (head == NULL) {
-head = g_malloc0(sizeof(*caps));
-caps = head;
-} else {
-caps->next = g_malloc0(sizeof(*caps));
-caps = caps->next;
-}
-caps->value =
-g_malloc(sizeof(*caps->value));
-caps->value->capability = i;
-caps->value->state = s->enabled_capabilities[i];
+caps = g_malloc(sizeof(*caps));
While being here, probably better use g_malloc0, for safety in future
+caps->capability = i;
+caps->state = s->enabled_capabilities[i];
+QAPI_LIST_APPEND(tail, caps);
}
return head;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ed4131efbca6..a9643ff41961 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1705,7 +1705,8 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
void hmp_sendkey(Monitor *mon, const QDict *qdict)
{
const char *keys = qdict_get_str(qdict, "keys");
-KeyValueList *keylist, *head = NULL, *tmp = NULL;
+KeyValue *v = NULL;
+KeyValueList *head = NULL, **tail =
int has_hold_time = qdict_haskey(qdict, "hold-time");
int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
Error *err = NULL;
@@ -1722,16 +1723,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
keyname_len = 4;
}
-keylist = g_malloc0(sizeof(*keylist));
-keylist->value = g_malloc0(sizeof(*keylist->value));
-
-if (!head) {
-head = keylist;
-}
-if (tmp) {
-tmp->next = keylist;
-}
-tmp = keylist;
+v = g_malloc0(sizeof(*v));
if (strstart(keys, "0x", NULL)) {
char *endp;
@@ -1740,16 +1732,18 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
if (endp != keys + keyname_len) {
goto err_out;
}
-keylist->value->type = KEY_VALUE_KIND_NUMBER;
-keylist->value->u.number.data = value;
+v->type = KEY_VALUE_KIND_NUMBER;
+v->u.number.data = value;
} else {
int idx = index_from_key(keys, keyname_len);
if (idx == Q_KEY_CODE__MAX) {
goto err_out;
}
-keylist->value->type = KEY_VALUE_KIND_QCODE;
-keylist->value->u.qcode.data = idx;
+v->type = KEY_VALUE_KIND_QCODE;
+v->u.qcode.data = idx;
}
+QAPI_LIST_APPEND(tail, v);
+v = NULL;
if (!*separator) {
break;
@@ -1761,6 +1755,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, err);
out:
+qapi_free_KeyValue(v);
alternative would be to define v as g_autoptr inside while-loop body and use
g_steal_pointer() for QAPI_LIST_APPEND().
qapi_free_KeyValueList(head);
return;
[..]
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a5058a3bd15e..10ee740eee1b 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2119,17 +2119,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,
@@ -2198,7 +2198,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 =
struct ifaddrs *ifap, *ifa;
if (getifaddrs() < 0) {
@@
[PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND
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
---
block/gluster.c| 13 ++
block/qapi.c | 14 +-
dump/dump.c| 22 +++--
hw/core/machine-qmp-cmds.c | 93 -
hw/mem/memory-device.c | 12 +
hw/pci/pci.c | 60
migration/migration.c | 20 +++-
monitor/hmp-cmds.c | 25 --
net/net.c | 13 +-
qga/commands-posix.c | 94 ++
qga/commands-win32.c | 88 ---
softmmu/tpm.c | 38 +++
ui/spice-core.c| 27 ---
13 files changed, 170 insertions(+), 349 deletions(-)
diff --git a/block/gluster.c b/block/gluster.c
index 1f8699b93835..e8ee14c8e9bf 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 **tail;
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);
+tail = >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(tail, gsconf);
gsconf = NULL;
qobject_unref(backing_options);
diff --git a/block/qapi.c b/block/qapi.c
index 3a1186fdccf5..0a96099e36e2 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -198,7 +198,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
{
int i, sn_count;
QEMUSnapshotInfo *sn_tab = NULL;
-SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
+SnapshotInfoList *head = NULL, **tail =
SnapshotInfo *info;
sn_count = bdrv_snapshot_list(bs, _tab);
@@ -233,17 +233,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
info->icount= sn_tab[i].icount;
info->has_icount= sn_tab[i].icount != -1ULL;
-info_list = g_new0(SnapshotInfoList, 1);
-info_list->value = info;
-
-/* XXX: waiting for the qapi to support qemu-queue.h types */
-if (!cur_item) {
-head = cur_item = info_list;
-} else {
-cur_item->next = info_list;
-cur_item = info_list;
-}
-
+QAPI_LIST_APPEND(tail, info);
}
g_free(sn_tab);
diff --git a/dump/dump.c b/dump/dump.c
index dec32468d98c..929138e91d08 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2030,39 +2030,29 @@ void qmp_dump_guest_memory(bool paging, const char
*file,
DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
{
-DumpGuestMemoryFormatList *item;
DumpGuestMemoryCapability *cap =
g_malloc0(sizeof(DumpGuestMemoryCapability));
+DumpGuestMemoryFormatList **tail = >formats;
/* elf is always available */
-item = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-cap->formats = item;
-item->value = DUMP_GUEST_MEMORY_FORMAT_ELF;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
/* kdump-zlib is always available */
-item->next = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-item = item->next;
-item->value = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB);
/* add new item if kdump-lzo is available */
#ifdef CONFIG_LZO
-item->next = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-item = item->next;
-item->value = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO);
#endif
/* add new item if kdump-snappy is available */
#ifdef CONFIG_SNAPPY
-item->next = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-item = item->next;
-item->value = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY);
#endif
/* Windows dump is available only if target is x86_64 */
#ifdef TARGET_X86_64
-item->next = g_malloc0(sizeof(DumpGuestMemoryFormatList));
-item = item->next;
-item->value = DUMP_GUEST_MEMORY_FORMAT_WIN_DMP;
+
