Re: [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces

[Olga Krishtal <okrishtal@parallels.com>]

On 01/04/15 18:39, Kirk Allan wrote:
> By default, IP addresses and prefixes will be derived from information
> obtained by various calls and structures.  IPv4 prefixes can be found
> by matching the address to those returned by GetApaptersInfo.  IPv6
> prefixes can not be matched this way due to the unpredictable order of
> entries.
GetAdaptersInfo.
> In Windows Visa/2008 guests and newer, it is possible to use inet_ntop()
> and  OnLinkPrefixLength to get  IPv4 and IPv6 addresses and prefixes.
> Setting –extra-cflags in the build configuration to
> ”- D_WIN32_WINNT-0x600 -DWINVER=0x600” or greater enables this functionality
> for those guests.  Setting –ectra-cflags is not required and if not used,
> the default approach will be taken.
>
> Signed-off-by: Kirk Allan <kallan@suse.com>
> ---
>   qga/commands-win32.c | 319 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 317 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3ef0549..635d3ca 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -16,11 +16,17 @@
>   #include <powrprof.h>
>   #include <stdio.h>
>   #include <string.h>
> +#include <winsock2.h>
> +#include <ws2tcpip.h>
> +#include <ws2ipdef.h>
> +#include <iptypes.h>
> +#include <iphlpapi.h>
>   #include "qga/guest-agent-core.h"
>   #include "qga/vss-win32.h"
>   #include "qga-qmp-commands.h"
>   #include "qapi/qmp/qerror.h"
>   #include "qemu/queue.h"
> +#include "qemu/host-utils.h"
>   
>   #ifndef SHTDN_REASON_FLAG_PLANNED
>   #define SHTDN_REASON_FLAG_PLANNED 0x80000000
> @@ -589,9 +595,318 @@ void qmp_guest_suspend_hybrid(Error **errp)
>       error_set(errp, QERR_UNSUPPORTED);
>   }
>   
> +#define WORKING_BUFFER_SIZE 15000
> +#define MAX_ALLOC_TRIES 2
> +
> +static DWORD guest_get_adapter_addresses(IP_ADAPTER_ADDRESSES **adptr_addrs)
> +{
> +    ULONG out_buf_len = 0;
> +    int alloc_tries = 0;
> +    DWORD ret = ERROR_SUCCESS;
> +
> +    /* Allocate a 15 KB buffer to start with.  If not enough, out_buf_len
> +     * will have the amount needed after the call to GetAdaptersAddresses.
> +     */
may be we should not allocate memory in the beginning and just make a 
call with just pointer,
and get necessary size.
> +    out_buf_len = WORKING_BUFFER_SIZE;
> +
> +    do {
> +        *adptr_addrs = (IP_ADAPTER_ADDRESSES *) g_malloc(out_buf_len);
> +        if (*adptr_addrs == NULL) {
> +            return ERROR_NOT_ENOUGH_MEMORY;
I think we should use error handling with win32 macro error_setg_win32( 
errno, GetLastError()..
> +        }
> +
> +        ret = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
> +            NULL, *adptr_addrs, &out_buf_len);
> +
> +        if (ret == ERROR_BUFFER_OVERFLOW) {
> +            g_free(*adptr_addrs);
> +            *adptr_addrs = NULL;
> +            alloc_tries++;
> +        }
> +
> +    } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries < MAX_ALLOC_TRIES));
Why we do it twice?
> +    if (ret != ERROR_SUCCESS) {
> +        if (*adptr_addrs) {
> +            g_free(*adptr_addrs);
> +            *adptr_addrs = NULL;
> +        }
> +    }
In this case we always have ERROR_SUCCESS.
> +    return ret;
> +}
> +
The comments for this function is the same. Moreover, they look very 
similar,
can we merge them and have #if (_WIN32_WINNT < 0x0600) inside the 
function body?
> +#if (_WIN32_WINNT < 0x0600)
> +static DWORD guest_get_adapters_info(IP_ADAPTER_INFO **adptr_info)
> +{
> +    ULONG out_buf_len = 0;
> +    int alloc_tries = 0;
> +    DWORD ret = ERROR_SUCCESS;
> +
> +    out_buf_len = sizeof(IP_ADAPTER_INFO);
> +    do {
> +        *adptr_info = (IP_ADAPTER_INFO *)g_malloc(out_buf_len);
> +        if (*adptr_info == NULL) {
> +            return ERROR_NOT_ENOUGH_MEMORY;
> +        }
> +
> +        ret = GetAdaptersInfo(*adptr_info, &out_buf_len);
> +
> +        if (ret == ERROR_BUFFER_OVERFLOW) {
> +            g_free(*adptr_info);
> +            *adptr_info = NULL;
> +            alloc_tries++;
> +        }
> +
> +    } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries < MAX_ALLOC_TRIES));
> +
> +    if (ret != ERROR_SUCCESS) {
> +        if (*adptr_info) {
> +            g_free(*adptr_info);
> +            *adptr_info = NULL;
> +        }
> +    }
> +    return ret;
> +}
> +#endif
> +static char *guest_wcstombs_dup(WCHAR *wstr)
> +{
> +    char *str;
> +    int i;
> +
> +    i = wcslen(wstr) + 1;
> +    str = g_malloc(i);
> +    if (str) {
This is Windows-specific part of qemu-ga, so we should win32 API use 
WideCharToMultiByte.
I am not sure that wcstombs uses Unicode UTF-16 and afaik it is deprecated.
> +        wcstombs(str, wstr, i);
> +    }
> +    return str;
> +}
> +
> +static char *guest_inet_ntop(int af, void *cp, char *buf, socklen_t len)
> +{
> +#if (_WIN32_WINNT >= 0x0600) && defined(ARCH_x86_64)
> +    /* If built for 64 bit Windows Vista/2008 or newer, inet_ntop() is
> +     * available for use.  Otherwise, do our best to derive it.
> +     */
> +    return (char *)inet_ntop(af, cp, buf, len);
> +#else
> +    u_char *p;
> +
> +    p = (u_char *)cp;
> +    if (af == AF_INET) {
May be we should do some struct IP_addr that will hold this information 
in appropriate format.
> +        snprintf(buf, len, "%u.%u.%u.%u", p[0], p[1], p[2], p[3]);
> +    } else if (af == AF_INET6) {
> +        int i, compressed_zeros, added_to_buf;
> +        char t[sizeof "::ffff"];
> +
> +        buf[0] = '\0';
> +        compressed_zeros = 0;
> +        added_to_buf = 0;
> +        for (i = 0; i < 16; i += 2) {
> +            if (p[i] != 0 || p[i + 1] != 0) {
> +                if (compressed_zeros) {
> +                    if (len > sizeof "::") {
> +                        strcat(buf, "::");
> +                        len -= sizeof "::" - 1;
> +                    }
> +                    added_to_buf++;
> +                } else {
> +                    if (added_to_buf) {
> +                        if (len > 1) {
> +                            strcat(buf, ":");
> +                            len--;
> +                        }
> +                    }
> +                    added_to_buf++;
> +                }
> +
> +                /* Take into account leading zeros. */
> +                if (p[i]) {
> +                    len -= snprintf(t, sizeof(t), "%x%02x", p[i], p[i+1]);
> +                } else {
> +                    len -= snprintf(t, sizeof(t), "%x", p[i+1]);
> +                }
> +
> +                /* Ensure there's enough room for the NULL. */
> +                if (len > 0) {
> +                    strcat(buf, t);
> +                }
> +                compressed_zeros = 0;
> +            } else {
> +                compressed_zeros++;
> +            }
> +        }
> +        if (compressed_zeros && !added_to_buf) {
> +            /* The address was all zeros. */
> +            strcat(buf, "::");
> +        }
> +    }
> +    return buf;
> +#endif
> +}
> +
> +static int64_t guest_ip_prefix(IP_ADAPTER_UNICAST_ADDRESS *ip_addr)
> +{
> +#if (_WIN32_WINNT >= 0x0600)
> +    /* If built for Windows Vista/2008 or newer, use the OnLinkPrefixLength
> +     * field to obtain the prefix.  Otherwise, do our best to figure it out.
> +     */
> +    return ip_addr->OnLinkPrefixLength;
> +#else
> +    int64_t prefix = -1; /* Use for AF_INET6 and unknown/undetermined values. */
> +
> +    if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
> +        IP_ADAPTER_INFO *adptr_info, *info;
> +        IP_ADDR_STRING *ip;
> +        struct in_addr *p;
> +
> +        if (guest_get_adapters_info(&adptr_info) != ERROR_SUCCESS) {
> +            return prefix;
> +        }
> +
> +        /* Match up the passed in ip_addr with one found in adaptr_info.
> +         * The matching one in adptr_info will have the netmask.
> +         */
> +        p = &((struct sockaddr_in *)ip_addr->Address.lpSockaddr)->sin_addr;
> +        for (info = adptr_info; info && prefix == -1; info = info->Next) {
> +            for (ip = &info->IpAddressList; ip; ip = ip->Next) {
> +                if (p->S_un.S_addr == inet_addr(ip->IpAddress.String)) {
> +                    prefix = ctpop32(inet_addr(ip->IpMask.String));
> +                    break;
> +                }
> +            }
> +        }
> +        g_free(adptr_info);
> +    }
> +    return prefix;
> +#endif
> +}
> +
>   GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>   {
> -    error_set(errp, QERR_UNSUPPORTED);
> +    IP_ADAPTER_ADDRESSES *adptr_addrs, *addr;
> +    GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
> +    GuestIpAddressList *head_addr, *cur_addr;
> +    DWORD ret;
> +
> +    ret = guest_get_adapter_addresses(&adptr_addrs);
> +    if (ret != ERROR_SUCCESS) {
> +        error_setg(errp, "failed to get adapter addresses %lu", ret);
> +        return NULL;
> +    }
> +
> +    for (addr = adptr_addrs; addr; addr = addr->Next) {
> +        GuestNetworkInterfaceList *info;
> +        GuestIpAddressList *address_item = NULL;
> +        char addr4[INET_ADDRSTRLEN];
> +        char addr6[INET6_ADDRSTRLEN];
> +        unsigned char *mac_addr;
> +        void *p;
Variables should not be declared in function body. I think better way is 
to put them in the beginning.
> +        IP_ADAPTER_UNICAST_ADDRESS *ip_addr = NULL;
> +
> +        info = g_malloc0(sizeof(*info));
> +        if (!info) {
> +            error_setg(errp, "failed to alloc a network interface list");
> +            goto error;
> +        }
> +
> +        if (!cur_item) {
> +            head = cur_item = info;
> +        } else {
> +            cur_item->next = info;
> +            cur_item = info;
> +        }
> +
> +        info->value = g_malloc0(sizeof(*info->value));
> +        if (!info->value) {
> +            error_setg(errp, "failed to alloc a network interface");
> +            goto error;
> +        }
> +        info->value->name = guest_wcstombs_dup(addr->FriendlyName);
> +
> +        if (addr->PhysicalAddressLength) {
> +            mac_addr = addr->PhysicalAddress;
> +
> +            info->value->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;
> +        }
> +
> +        head_addr = NULL;
> +        cur_addr = NULL;
> +        for (ip_addr = addr->FirstUnicastAddress;
> +                ip_addr;
> +                ip_addr = ip_addr->Next) {
> +            address_item = g_malloc0(sizeof(*address_item));
> +            if (!address_item) {
> +                error_setg(errp, "failed to alloc an Ip address list");
> +                goto error;
> +            }
> +
> +            if (!cur_addr) {
> +                head_addr = cur_addr = address_item;
> +            } else {
> +                cur_addr->next = address_item;
> +                cur_addr = address_item;
> +            }
> +
> +            address_item->value = g_malloc0(sizeof(*address_item->value));
> +            if (!address_item->value) {
> +                error_setg(errp, "failed to alloc an Ip address");
> +                goto error;
> +            }
> +
> +            if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
> +                p = &((struct sockaddr_in *)
> +                    ip_addr->Address.lpSockaddr)->sin_addr;
> +
> +                if (!guest_inet_ntop(AF_INET, p, addr4, sizeof(addr4))) {
> +            		error_setg(errp,
win32 error macro
> +                               "failed address presentation form conversion");
> +                    goto error;
> +                }
> +
> +                address_item->value->ip_address = g_strdup(addr4);
> +                address_item->value->ip_address_type =
> +                    GUEST_IP_ADDRESS_TYPE_IPV4;
> +            } else if (ip_addr->Address.lpSockaddr->sa_family == AF_INET6) {
> +                p = &((struct sockaddr_in6 *)
> +                    ip_addr->Address.lpSockaddr)->sin6_addr;
> +
> +                if (!guest_inet_ntop(AF_INET6, p, addr6, sizeof(addr6))) {
> +                    error_setg(errp,
> +                               "failed address presentation form conversion");
win32 error macro
>                      goto error;
> +                }
> +
> +                address_item->value->ip_address = g_strdup(addr6);
> +                address_item->value->ip_address_type =
> +                    GUEST_IP_ADDRESS_TYPE_IPV6;
> +            }
> +            address_item->value->prefix = guest_ip_prefix(ip_addr);
> +        }
> +        if (head_addr) {
> +            info->value->has_ip_addresses = true;
> +            info->value->ip_addresses = head_addr;
> +        }
> +    }
> +
> +    if (adptr_addrs) {
> +        g_free(adptr_addrs);
> +    }
> +    return head;
> +
> +error:
> +    if (adptr_addrs) {
> +        g_free(adptr_addrs);
> +    }
> +    if (head) {
> +        qapi_free_GuestNetworkInterfaceList(head);
> +    }
>       return NULL;
>   }
>   
> @@ -707,7 +1022,7 @@ GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
>   GList *ga_command_blacklist_init(GList *blacklist)
>   {
>       const char *list_unsupported[] = {
> -        "guest-suspend-hybrid", "guest-network-get-interfaces",
> +        "guest-suspend-hybrid",
>           "guest-get-vcpus", "guest-set-vcpus",
>           "guest-set-user-password",
>           "guest-get-memory-blocks", "guest-set-memory-blocks",
I think 2 last functions  GuestNetworkInterfaceList 
*qmp_guest_network_get_interfaces(Error **errp) and  static char 
*guest_inet_ntop(int af, void *cp, char *buf, socklen_t len)should be 
redesigned and refactored very attentively. Secondly you should decide 
what kind of functions to use Win32 API or POSIX API.
It is bed idea to mix them.
Pls, pay attention to to error handling.