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

["Denis V. Lunev" <den@openvz.org>]

On 28/05/15 21:41, 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 GetAdaptersInfo.  IPv6
> prefixes can not be matched this way due to the unpredictable order of
> entries.
>
> 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 | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 290 insertions(+), 2 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3ef0549..3bf9011 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,291 @@ void qmp_guest_suspend_hybrid(Error **errp)
>       error_set(errp, QERR_UNSUPPORTED);
>   }
>
> +static DWORD guest_get_adapters_addresses(IP_ADAPTER_ADDRESSES **adptr_addrs)
> +{
> +    ULONG adptr_addrs_len = 0;
> +    DWORD ret = ERROR_SUCCESS;
> +
> +    /* Call the first time to get the adptr_addrs_len. */
> +    *adptr_addrs = NULL;
> +    GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
> +                         NULL, *adptr_addrs, &adptr_addrs_len);
> +
> +    *adptr_addrs = g_malloc(adptr_addrs_len);
> +    if (*adptr_addrs == NULL) {
> +        ret = ERROR_NOT_ENOUGH_MEMORY;
> +    } else {
> +        ret = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
> +                                   NULL, *adptr_addrs, &adptr_addrs_len);
> +        if (ret != ERROR_SUCCESS) {
> +            g_free(*adptr_addrs);
> +            *adptr_addrs = NULL;
> +        }
> +    }
> +    return ret;

https://developer.gnome.org/glib/2.28/glib-Memory-Allocation.html#g-malloc

g_malloc never fail

If any call to allocate memory fails, the application is terminated. 
This also means that there is no need to check if the call succeeded.

this means that IP_ADAPTER_ADDRESSES could be returned from function

> +}
> +
> +#if (_WIN32_WINNT < 0x0600)
are you correct with the condition? according to MSDN

On Windows XP and later:  Use the GetAdaptersAddresses function instead 
of GetAdaptersInfo.

Thus you should have above branch of code undefined.


> +static DWORD guest_get_adapters_info(IP_ADAPTER_INFO **adptr_info)
> +{
> +    ULONG adptr_info_len = 0;
> +    DWORD ret = ERROR_SUCCESS;
> +
> +    /* Call the first time to get the adptr_info_len. */
> +    *adptr_info = NULL;
> +    GetAdaptersInfo(*adptr_info, &adptr_info_len);
> +
> +    *adptr_info = g_malloc(adptr_info_len);
> +    if (*adptr_info == NULL) {
> +        ret = ERROR_NOT_ENOUGH_MEMORY;
> +    } else {
> +        ret = GetAdaptersInfo(*adptr_info, &adptr_info_len);
> +        if (ret != ERROR_SUCCESS) {
> +            g_free(*adptr_info);
> +            *adptr_info = NULL;
> +        }
> +    }

same note about the allocation

> +    return ret;
> +}
> +#endif
> +
> +static char *guest_wctomb_dup(WCHAR *wstr)
> +{
> +    char *str;
> +    size_t i;
> +
> +    i = wcslen(wstr) + 1;
> +    str = g_malloc(i);
> +    if (str) {
> +        WideCharToMultiByte(CP_ACP, WC_COMPOSITECHECK,
> +                            wstr, -1, str, i, NULL, NULL);
> +    }

same allocation

> +    return str;
> +}
> +
> +static char *guest_inet_ntop(int af, void *cp, char *buf, size_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.
> +     */

nothing about 64bit only is present here. This seems strange to me
https://msdn.microsoft.com/ru-ru/library/windows/desktop/cc805843(v=vs.85).aspx

> +    return (char *)InetNtop(af, cp, buf, len);
> +#else
> +    u_char *p;
> +
> +    p = (u_char *)cp;
> +    if (af == AF_INET) {
> +        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"];

sizeof without braces and from string could be tricky but I am not
quite sure

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

with len == 1 you will definitely have problem with wrong conversion

> +                    }
> +                    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]);
> +                }

snprintf can produce non-zero terminated character arrays
and the same problem with length...

> +
> +                /* 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, "::");
there is not length check here

> +        }
> +    }
> +    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. */
> +    IP_ADAPTER_INFO *adptr_info, *info;
> +    IP_ADDR_STRING *ip;
> +    struct in_addr *p;
> +
> +    if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {

why not to return immediately? You will have 1 less indent

> +        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;
why not goto here. Moving out from inner loop with goto is a good thing.
There is no necessity to use prefix == -1 above in this case

> +                }
> +            }
> +        }
> +        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;
> +    IP_ADAPTER_UNICAST_ADDRESS *ip_addr = NULL;
> +    GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
> +    GuestIpAddressList *head_addr, *cur_addr;
> +    GuestNetworkInterfaceList *info;
> +    GuestIpAddressList *address_item = NULL;
> +    unsigned char *mac_addr;
> +    void *p;
> +    char addr4[INET_ADDRSTRLEN];
> +    char addr6[INET6_ADDRSTRLEN];
> +    DWORD ret;
> +
> +    ret = guest_get_adapters_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) {
> +        info = g_malloc0(sizeof(*info));
> +        if (!info) {

no need to check, see above

> +            error_setg(errp, "failed to alloc a network interface list");
> +            goto error;
> +        }
> +
> +        if (!cur_item) {
it is better to reserve ! condition to logical checks. For pointers it 
is better to check using != NULL


> +            head = cur_item = info;
> +        } else {
> +            cur_item->next = info;
> +            cur_item = info;
> +        }
> +
> +        info->value = g_malloc0(sizeof(*info->value));
> +        if (!info->value) {
same

> +            error_setg(errp, "failed to alloc a network interface");
> +            goto error;
> +        }
> +        info->value->name = guest_wctomb_dup(addr->FriendlyName);
> +
> +        if (addr->PhysicalAddressLength) {

same for check

> +            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_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_win32(errp, WSAGetLastError(),
> +                        "failed address presentation form conversion");
> +                    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);

if is not needed

https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-free

> +    }
> +    return head;
> +
> +error:
> +    if (adptr_addrs) {
> +        g_free(adptr_addrs);
> +    }
> +    if (head) {
> +        qapi_free_GuestNetworkInterfaceList(head);
> +    }
>       return NULL;
>   }
>
> @@ -707,7 +995,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",
>