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

[Kenth Andersson]

I will take care of the below items as soon as I get some free time.

/Kenth

On 01 Oct 2014, at 19:45, Michael Roth <address@hidden> wrote:

> 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)
> 
>