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

[Kirk Allan]

Thanks for the feedback.  I'll make the necessary adjustments and create a new 
patch.

Kirk

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

I'll fix the typo.

>> 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 <address@hidden>
>> ---
>>   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.

I'll get the size first and make one allocation.

>> +    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()..

I'll use error_setg_win32() when the api sets last error.

>> +        }
>> +
>> +        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?

I'll do the same as above, get the size first and do one allocation.  Since we 
are doing GetAdaptersAddresses and GetAdaptersInfo, I don't see usable way in 
combining these two.  Simplifying th
e memory allocation should help though.

>> +#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.

Will switch to WideCharToMultiByte.

>> +        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.

inet_ntop just returns a string, so I'm not sure what else could be done here.

>> +        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.

I'll move the variable to 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

Since the error can be retrieved with WSAGetLastErorr, I'll switch to 
error_setg_win32(errp, WSAGetLastError(), ...

>> +                               "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

Same as above.

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

Where InetNtop is available where inet_ntop is, I'll switch the win32 api.

> Pls, pay attention to to error handling.

For apis that can retrieve the error form GetLastError, I'll switch to the 
error_setg_win32.