Re: [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@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@redhat.com>
> ---
[...]
> 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) {

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(&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;
> +        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;
>              }
>          }
>      }
[...]