[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