[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND
|
From: |
Vladimir Sementsov-Ogievskiy |
|
Subject: |
Re: [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND |
|
Date: |
Thu, 24 Dec 2020 14:35:55 +0300 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
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 <eblake@redhat.com>
---
[..]
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 = &head;
+ 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 = &head;
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 = &head;
struct ifaddrs *ifap, *ifa;
if (getifaddrs(&ifap) < 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 = &info->ip_addresses;
+
+ if (!info->has_hardware_address && ifa->ifa_flags & SIOCGIFHWADDR) {
/* 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(&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",
@@ -2256,13 +2252,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 &&
@@ -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;
- 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) {
@@ -2295,15 +2290,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]) +
@@ -2315,29 +2309,18 @@ GuestNetworkInterfaceList
*qmp_guest_network_get_interfaces(Error **errp)
continue;
}
- address_list = &info->value->ip_addresses;
address_tail is used only here, I'd prefere it to be initialized here.
+ QAPI_LIST_APPEND(address_tail, address_item);
- while (*address_list && (*address_list)->next) {
- address_list = &(*address_list)->next;
- }
Hmm. It's wrong. Original code searches for the end of the list, but with the
patch we just APPEND to the head of non-empty list.
As I understand, list may be non-empty if info comes from guest_find_interface,
that's why this loop is here.
+ info->has_ip_addresses = true;
- if (!*address_list) {
- *address_list = address_item;
- } else {
- (*address_list)->next = address_item;
- }
-
- info->value->has_ip_addresses = true;
-
- if (!info->value->has_statistics) {
+ if (!info->has_statistics) {
So, with squashed-in:
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 10ee740eee..c4815d4b0d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2229,8 +2229,6 @@ GuestNetworkInterfaceList
*qmp_guest_network_get_interfaces(Error **errp)
QAPI_LIST_APPEND(tail, info);
}
- address_tail = &info->ip_addresses;
-
if (!info->has_hardware_address && ifa->ifa_flags & SIOCGIFHWADDR) {
/* we haven't obtained HW address yet */
sock = socket(PF_INET, SOCK_STREAM, 0);
@@ -2309,6 +2307,10 @@ GuestNetworkInterfaceList
*qmp_guest_network_get_interfaces(Error **errp)
continue;
}
+ address_tail = &info->ip_addresses;
+ while (!*address_tail) {
+ address_tail = &(*address_tail)->next;
+ }
QAPI_LIST_APPEND(address_tail, address_item);
info->has_ip_addresses = true;
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
- [PATCH v3 0/7] Common macros for QAPI list growth, Eric Blake, 2020/12/23
- [PATCH v3 2/7] rocker: Revamp fp_port_get_info, Eric Blake, 2020/12/23
- [PATCH v3 3/7] migration: Refactor migrate_cap_add, Eric Blake, 2020/12/23
- [PATCH v3 1/7] net: Clarify early exit condition, Eric Blake, 2020/12/23
- [PATCH v3 5/7] qapi: Introduce QAPI_LIST_APPEND, Eric Blake, 2020/12/23
- [PATCH v3 6/7] qapi: Use QAPI_LIST_APPEND in trivial cases, Eric Blake, 2020/12/23
- [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND, Eric Blake, 2020/12/23
- Re: [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND,
Vladimir Sementsov-Ogievskiy <=
- [PATCH v3 4/7] qapi: Use QAPI_LIST_PREPEND() where possible, Eric Blake, 2020/12/23