Re: [Qemu-devel] [PATCH v2] qemu-ga: added windows support for 'guest-network-get-interfaces'

[Michael Roth]

Quoting Kenth Andersson (2014-09-29 13:22:54)
> Implementation of guest-network-get-interfaces for windows
> 
> 
> Signed-off-by: Kenth Andersson <address@hidden>

Thanks! I've been testing the functionality and it seems work nicely. Some
review comments below though:

> ---
>  configure            |   2 +-
>  qga/commands-win32.c | 247 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 244 insertions(+), 5 deletions(-)
> 
> diff --git a/configure b/configure
> index 681abfc..7c6c60c 100755
> --- a/configure
> +++ b/configure
> @@ -717,7 +717,7 @@ EOF
>    sysconfdir="\${prefix}"
>    local_statedir=
>    confsuffix=""
> -  libs_qga="-lws2_32 -lwinmm -lpowrprof $libs_qga"
> +  libs_qga="-lws2_32 -lwinmm -lpowrprof -liphlpapi $libs_qga"
>  fi
>  
>  werror=""
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3bcbeae..fb6fdba 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -14,6 +14,9 @@
>  #include <glib.h>
>  #include <wtypes.h>
>  #include <powrprof.h>
> +#include <winsock2.h>
> +#include <iphlpapi.h>
> +#include <ws2tcpip.h>
>  #include "qga/guest-agent-core.h"
>  #include "qga/vss-win32.h"
>  #include "qga-qmp-commands.h"
> @@ -29,6 +32,9 @@
>                         (365 * (1970 - 1601) +       \
>                          (1970 - 1601) / 4 - 3))
>  
> +/* Defines the max length of an IPv6 address in human readable format + pad 
> */
> +#define MAX_IPv6_LEN 50
> +
>  static void acquire_privilege(const char *name, Error **errp)
>  {
>      HANDLE token = NULL;
> @@ -359,10 +365,244 @@ void qmp_guest_suspend_hybrid(Error **errp)
>      error_set(errp, QERR_UNSUPPORTED);
>  }
>  
> +static int get_prefix_length(PIP_ADAPTER_PREFIX prefix,
> +                      SOCKET_ADDRESS *sockaddr, int socklen)
> +{
> +    PIP_ADAPTER_PREFIX p;
> +    struct sockaddr *addr = sockaddr->lpSockaddr;
> +
> +    for (p = prefix; p; p = p->Next) {
> +        /* Compare the ip-adderss address family with the prefix
> +         * address family */
> +        if (addr->sa_family == p->Address.lpSockaddr->sa_family) {
> +            int num_bytes = p->PrefixLength / 8;
> +            unsigned char *src = 0;
> +            unsigned char *dst = 0;
> +            int remaining_bits;
> +
> +            if (addr->sa_family == AF_INET) {
> +                struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> +                src = (unsigned char *)&(sin->sin_addr.s_addr);
> +            } else if (addr->sa_family == AF_INET6) {
> +                struct sockaddr_in6 *sin = (struct sockaddr_in6 *)addr;
> +                src = (unsigned char *)sin->sin6_addr.s6_addr;
> +            }
> +
> +            if (p->Address.lpSockaddr->sa_family == AF_INET) {
> +                struct sockaddr_in *sin =
> +                            (struct sockaddr_in *)p->Address.lpSockaddr;
> +                dst = (unsigned char *)&(sin->sin_addr.s_addr);
> +            } else if (p->Address.lpSockaddr->sa_family == AF_INET6) {
> +                struct sockaddr_in6 *sin =
> +                            (struct sockaddr_in6 *)p->Address.lpSockaddr;
> +                dst = (unsigned char *)sin->sin6_addr.s6_addr;
> +            }
> +
> +            /* If non of the addresses are in correct format we will continue
> +             * to next one */
> +            if (src == 0 || dst == 0) {
> +                continue;
> +            }
> +
> +            /* Check if prefix network is the same network as we are testing
> +             * start with whole bytes */
> +
> +            if (memcmp(src, dst, num_bytes) != 0) {
> +                continue;
> +            }
> +
> +            /* Check the remaning bits */
> +            remaining_bits = p->PrefixLength % 8;
> +
> +            if (remaining_bits != 0) {
> +                unsigned char mask = 0xff << (8 - remaining_bits);
> +                int i = num_bytes;
> +
> +                if ((src[i] & mask) != (dst[i] & mask)) {
> +                    continue;
> +                }
> +            }
> +
> +            return p->PrefixLength;
> +        }
> +    }
> +    return 0;
> +}
> +
> +
> +
>  GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  {
> -    error_set(errp, QERR_UNSUPPORTED);
> -    return NULL;
> +    GuestNetworkInterfaceList *head = NULL, *curr_item = NULL;
> +    DWORD ret_val;
> +
> +    PIP_ADAPTER_ADDRESSES adpt_addrs;
> +    PIP_ADAPTER_ADDRESSES curr_adpt_addrs;
> +    PIP_ADAPTER_UNICAST_ADDRESS adpt_uc_addr;
> +
> +    /* GetAdaptersAddresses requires ULONG */
> +    ULONG bufLen = sizeof(IP_ADAPTER_ADDRESSES);

buf_len

> +
> +    /* Startup WinSock32 */
> +    WORD ws_version_requested = MAKEWORD(2, 2);
> +    WSADATA ws_data;
> +    WSAStartup(ws_version_requested, &ws_data);

Since this can fail, we should return an error and bail (after calling 
WSACleanup).
Something like:

  error_setg(errp, "failed to initialize Windows Socket API v2.2");

> +
> +    /* Allocate adapter address list with one entry, used to
> +     * fetch the read length needed by GetAdapterAddresses */
> +    adpt_addrs = g_malloc(sizeof(IP_ADAPTER_ADDRESSES));
> +
> +    /* Get adapter adresses and if it doesn't fit in adpt_addrs

addresses

> +     * we will realloc */
> +    if (GetAdaptersAddresses(AF_UNSPEC,
> +                             GAA_FLAG_INCLUDE_PREFIX,
> +                             NULL,
> +                             adpt_addrs, &bufLen) ==
> +                                            ERROR_BUFFER_OVERFLOW) {
> +
> +        g_free(adpt_addrs);
> +        adpt_addrs = g_malloc(bufLen);
> +    }
> +
> +    /* Get adapter address list */
> +    ret_val = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
> +                                  NULL,
> +                                  adpt_addrs, &bufLen);

Doesn't this mean we end up calling GetAdaptersAddresses unconditionally 2
times? Why not only make the second attempt if you get the overflow and
need to resize adpt_addrs?

Alternatively, maybe you can call GetAdaptersAddresses once with a NULL buffer
just to probe the size. This would still be 2 calls, but you can avoid
needing to guess at an initial size for adpt_addrs.

> +    if (ret_val == NO_ERROR) {

I would just jump to error-handling if ret_val != NO_ERROR so you don't
need to indent the main block of code.

> +
> +        /* Iterate all adapter addresses */
> +        curr_adpt_addrs = adpt_addrs;
> +
> +        while (curr_adpt_addrs) {
> +            /* Check if this adapter is up and running */
> +
> +            if (curr_adpt_addrs->OperStatus == IfOperStatusUp &&
> +                (curr_adpt_addrs->IfType == IF_TYPE_ETHERNET_CSMACD ||
> +                 curr_adpt_addrs->IfType == IF_TYPE_IEEE80211 ||
> +                 curr_adpt_addrs->IfType == IF_TYPE_SOFTWARE_LOOPBACK)) {

Probably unlikely, but shouldn't IF_TYPE_PPP be included? And do we really
need to filter these at all? In the POSIX implementation we include all of
these, as well as IF_TYPE_TUNNEL (via tap/tun interfaces).

> +
> +                int len = 0;
> +                char *temp_description = 0;
> +
> +                GuestNetworkInterfaceList *interface_list =
> +                                 g_malloc0(sizeof(*interface_list));
> +                interface_list->value =
> +                                 g_malloc0(sizeof(*interface_list->value));

Small nit, but it gets a little to keep track of the pointers here. Maybe we
should just use sizeof(GuestNetworkInterfaceList) and
sizeof(GuestNetworkInterface) explicitly?

> +
> +                len = wcslen(curr_adpt_addrs->Description) + 1;
> +                temp_description = g_malloc(len);
> +
> +                wcstombs(temp_description,
> +                         curr_adpt_addrs->Description,
> +                         len);
> +
> +                interface_list->value->name = g_strdup(temp_description);
> +
> +                g_free(temp_description);
> +
> +                if (curr_item == NULL) {
> +                    head = curr_item = interface_list;
> +                } else {
> +                    curr_item->next = interface_list;
> +                    curr_item = interface_list;
> +                }
> +
> +                /* Format MAC address */
> +                interface_list->value->hardware_address =
> +                    g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
> +                       (unsigned int) curr_adpt_addrs->PhysicalAddress[0],
> +                       (unsigned int) curr_adpt_addrs->PhysicalAddress[1],
> +                       (unsigned int) curr_adpt_addrs->PhysicalAddress[2],
> +                       (unsigned int) curr_adpt_addrs->PhysicalAddress[3],
> +                       (unsigned int) curr_adpt_addrs->PhysicalAddress[4],
> +                       (unsigned int) curr_adpt_addrs->PhysicalAddress[5]);
> +
> +
> +                interface_list->value->has_hardware_address = true;
> +
> +                /* Iterate all unicast addresses of this adapter */
> +                GuestIpAddressList **address_list =
> +                                        &interface_list->value->ip_addresses;
> +
> +                GuestIpAddressList *address_item = NULL;
> +
> +                adpt_uc_addr = curr_adpt_addrs->FirstUnicastAddress;
> +
> +                while (adpt_uc_addr) {
> +                    SOCKET_ADDRESS *saddr = &adpt_uc_addr->Address;
> +                    int socklen;
> +                    char buffer[MAX_IPv6_LEN];
> +                    DWORD len = MAX_IPv6_LEN;
> +
> +                    /* Allocate an address item */
> +                    address_item = g_malloc0(sizeof(*address_item));
> +
> +                    address_item->value =
> +                                    g_malloc0(sizeof(*address_item->value));
> +
> +                    /* Is this IPv4 or IPv6 */
> +                    if (saddr->lpSockaddr->sa_family == AF_INET) {
> +                        address_item->value->ip_address_type =
> +                                                GUEST_IP_ADDRESS_TYPE_IPV4;
> +                        socklen = sizeof(struct sockaddr_in);
> +                    } else if (saddr->lpSockaddr->sa_family == AF_INET6) {
> +                        address_item->value->ip_address_type =
> +                                                GUEST_IP_ADDRESS_TYPE_IPV6;
> +                        socklen = sizeof(struct sockaddr_in6);
> +                    } else {
> +                        socklen = 0;

What's supposed to happen in this situation? Maybe we should just set
skip the address field in this case? And if we do, we should be carefully
about setting has_ip_addresses unconditionally below.

> +                    }
> +
> +                    /* Temporary buffer to hold human readable address */
> +                    WSAAddressToString(saddr->lpSockaddr,
> +                                       socklen, NULL, buffer, &len);
> +                    buffer[len] = 0;
> +
> +                    address_item->value->ip_address = g_strdup(buffer);
> +
> +                    /* Get the prefix for this address */
> +                    address_item->value->prefix =
> +                              get_prefix_length(curr_adpt_addrs->FirstPrefix,
> +                                         saddr, socklen);


> +
> +                    /* Add this address to the end of the address list */
> +                    while (*address_list && (*address_list)->next) {
> +                        address_list = &(*address_list)->next;
> +                    }
> +
> +                    if (!*address_list) {
> +                        *address_list = address_item;
> +                    } else {
> +                        (*address_list)->next = address_item;
> +                    }

Maybe something like this would be cleaner:

                    /* Iterate all unicast addresses of this adapter */
                    GuestIpAddressList *current_address_item = NULL;

                    adpt_uc_addr = curr_adpt_addrs->FirstUnicastAddress;

                    while (adpt_uc_addr) {
                    ...

                       if (!current_address_item) {
                           interface_list->value->ip_addresses = address_item;
                           current_address_item = address_item;
                       } else {
                           current_address_item->next = address_item;
                           current_address_item = address_item;
                       }
                    }

In fact, I just noticed that's already how you're handling
GuestNetworkInterfaceList above.

> +
> +                    /* Jump to next address */
> +                    adpt_uc_addr = adpt_uc_addr->Next;
> +                }
> +                interface_list->value->has_ip_addresses = true;
> +
> +            }
> +
> +            /* Jump to next adapter */
> +            curr_adpt_addrs = curr_adpt_addrs->Next;
> +        }
> +    }
> +
> +    /* Free the adapter list */
> +    if (adpt_addrs != NULL) {
> +        g_free(adpt_addrs);
> +    }
> +
> +    /* Cleanup WinSock32 */
> +    WSACleanup();
> +
> +    if (!head) {
> +        error_set(errp,
> +                  ERROR_CLASS_GENERIC_ERROR,
> +                  "Could not get network interface information");

you can use error_setg() for generic errors

> +    }
> +
> +    return head;
>  }
>  
>  int64_t qmp_guest_get_time(Error **errp)
> @@ -452,8 +692,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      const char *list_unsupported[] = {
>          "guest-file-open", "guest-file-close", "guest-file-read",
>          "guest-file-write", "guest-file-seek", "guest-file-flush",
> -        "guest-suspend-hybrid", "guest-network-get-interfaces",
> -        "guest-get-vcpus", "guest-set-vcpus",
> +        "guest-suspend-hybrid", "guest-get-vcpus", "guest-set-vcpus",
>          "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
>          "guest-fstrim", NULL};
>      char **p = (char **)list_unsupported;
> -- 
> 1.9.3 (Apple Git-50)