[GEDI] [PATCH v3 7/7] qapi: More complex uses of QAPI_LIST_APPEND
Markus Armbruster
armbru at redhat.com
Wed Jan 13 13:31:07 UTC 2021
Eric Blake <eblake at 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 at 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;
> }
> }
> }
[...]
More information about the integration
mailing list