[GEDI] [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND
Vladimir Sementsov-Ogievskiy
vsementsov at virtuozzo.com
Thu Dec 24 11:35:55 UTC 2020
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 at 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 at virtuozzo.com>
--
Best regards,
Vladimir
More information about the integration
mailing list