[Qemu-devel] [PATCH v3 0/2] qga: qmp_guest_network_get_interfaces for win32

[Qemu-devel] [PATCH v3 0/2] qga: qmp_guest_network_get_interfaces for win32

[Kirk Allan]

Changes from v2:
- Patch 1/2 has no changes.
- Patch 2/2 implemented feedback from v2. For GetAdaptersAddresses and
GetAdaptersInfo, made an initial call to get size required before allocating.
Use error_setg_win32 when Win32 API sets last error.  Use Win32 InetNtop rather
than inet_ntop.  Delcare variable at beginning of function rather than in a
block.

This patch set is to implement qmp_guest_network_get_interfaces for win32.

This version splits the previous single patch into two patches: configuration
and implementation.

The configuration patch utilizes the –extra-cflags rather than introduce
a new option for setting _WIN32_WINNT and WINVER.

The implementation patch for commands-win32.c is that same as before.
It will take advantage of _WIN32_WINNT if set to 0x600 or greater for
Windows Vista/2008 guests or newer to use inet_ntop and OnLinkPrefixLength
for getting addresses and prefixes.

Kirk Allan (2):
  qga: add additional win32 cflags and libraries
  qga: win32 implementation of qmp_guest_network_get_interfaces

 configure            |   9 +-
 qga/commands-win32.c | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 297 insertions(+), 4 deletions(-)

-- 
1.8.5.6

[Qemu-devel] [PATCH v3 1/2] qga: add additional win32 cflags and libraries

[Kirk Allan]

Use –extra-cflags to set cflags to such as _WIN32_WINVER and WINVER to
add additional functionality offered in Windows Visat/2008 and newer.

Add the -DARCH_$ARCH cflag.

Add the iphlpapi library to use APIs such as GetAdaptersInfo and
GetAdaptersAddresses.

Signed-off-by: Kirk Allan <kallan@suse.com>
---
 configure | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 9852aef..e654283 100755
--- a/configure
+++ b/configure
@@ -708,7 +708,12 @@ fi
 if test "$mingw32" = "yes" ; then
   EXESUF=".exe"
   DSOSUF=".dll"
-  QEMU_CFLAGS="-DWIN32_LEAN_AND_MEAN -DWINVER=0x501 $QEMU_CFLAGS"
+   # --extra-cflags can be used to set flags such as -DWINVER and
+   # -D_WIN32_WINNT.  If -DWINVER has not be set, default to XP (0x501).
+  if [ "$QEMU_CFLAGS" = "${QEMU_CFLAGS%-DWINVER=*}" ] ; then
+    QEMU_CFLAGS="-DWINVER=0x501 $QEMU_CFLAGS"
+  fi
+  QEMU_CFLAGS="-DWIN32_LEAN_AND_MEAN -DARCH_$ARCH $QEMU_CFLAGS"
   # enable C99/POSIX format strings (needs mingw32-runtime 3.15 or later)
   QEMU_CFLAGS="-D__USE_MINGW_ANSI_STDIO=1 $QEMU_CFLAGS"
   LIBS="-lwinmm -lws2_32 -liphlpapi $LIBS"
@@ -724,7 +729,7 @@ if test "$mingw32" = "yes" ; then
   sysconfdir="\${prefix}"
   local_statedir=
   confsuffix=""
-  libs_qga="-lws2_32 -lwinmm -lpowrprof $libs_qga"
+  libs_qga="-lws2_32 -lwinmm -lpowrprof -liphlpapi $libs_qga"
 fi
 
 werror=""
-- 
1.8.5.6

Re: [Qemu-devel] [PATCH v3 1/2] qga: add additional win32 cflags and libraries

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On 05/28/2015 12:41 PM, Kirk Allan wrote:
> Use –extra-cflags to set cflags to such as _WIN32_WINVER and WINVER to

That Unicode mdash looks suspicious; did you mean for it to be two ASCII
-- instead?

> add additional functionality offered in Windows Visat/2008 and newer.

s/Visat/Vista/

> 
> Add the -DARCH_$ARCH cflag.
> 
> Add the iphlpapi library to use APIs such as GetAdaptersInfo and
> GetAdaptersAddresses.
> 
> Signed-off-by: Kirk Allan <kallan@suse.com>
> ---
>  configure | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/2] qga: add additional win32 cflags and libraries

[Kirk Allan]

 >>>
> On 05/28/2015 12:41 PM, Kirk Allan wrote:
>> Use *extra-cflags to set cflags to such as _WIN32_WINVER and WINVER to
> 
> That Unicode mdash looks suspicious; did you mean for it to be two ASCII
> -- instead?
Your right, it is --extra-clfags.  I'll fix that.
> 
>> add additional functionality offered in Windows Visat/2008 and newer.
> 
> s/Visat/Vista/
I'll fix the typo.
> 
>> 
>> Add the -DARCH_$ARCH cflag.
>> 
>> Add the iphlpapi library to use APIs such as GetAdaptersInfo and
>> GetAdaptersAddresses.
>> 
>> Signed-off-by: Kirk Allan <kallan@suse.com>
>> ---
>>  configure | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

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

[Kirk Allan]

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;
+}
+
+#if (_WIN32_WINNT < 0x0600)
+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;
+        }
+    }
+    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);
+    }
+    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.
+     */
+    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"];
+
+        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. */
+    IP_ADAPTER_INFO *adptr_info, *info;
+    IP_ADDR_STRING *ip;
+    struct in_addr *p;
+
+    if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
+        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;
+    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) {
+            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_wctomb_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_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);
+    }
+    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",
-- 
1.8.5.6

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

[Denis V. Lunev]

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

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

[Olga Krishtal]

[-- Attachment #1: Type: text/plain, Size: 15893 bytes --]

On 28/05/15 23:54, Denis V. Lunev wrote:
> 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
I think that check is still necessary, because we may suffer some other 
problems:
*ERROR_ADDRESS_NOT_ASSOCIATED*
*ERROR_INVALID_PARAMETER*
and even *Other*, according to MSDN.

 From my point of view, the prototype of function should be smth like this:
static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses()
{

	PIP_ADAPTER_ADDRESSES ip_add = NULL;
	/* Memory allocation, etc */
	......
	if (GetAdaptersAddresses(....)) {
		error_setg_win32(errp, GetLastError(), "Failed smth");
		.....
	}

     return ip_add;

}
**
>
>> +}
>> +
>> +#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.
>
It is better to use #ifdef (smth), #else and #endif to separate win version.
Den is right, the first func will be compiled always, and the winxp 
variation
will be compiled for winserver 2003, and winxp. Have you tested if for 
2003 server?
>
>> +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
Same prototype as before.
>
>> +    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 
>
>
If you will use glib utility: nm_utils_inet4/6_ntop() you won't need 
this separation.
>> +    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
>> +}
>> +
I still recommend to think over this function, here useful link
https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html
glib provides a lot of function to work with strings, and there you do 
not need think
about memory allocations and etc. Pls, look.

>> +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;
>> +        }
adptr_info = guest_get_adapters_info();
if(adptr_info == NULL )
     goto out;
If you will use the prototypes, I have written before it will be easier.


>> +        /* 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;
>> +
May be you should return  pointer to structure by 
guest_get_adapters_addresses, and
make this function take void? As the result you won't need if (ret != 
ERROR_SUCCESS)
It will be smth like this:
adptr_addrs = guest_get_adapters_addresses();
if (adptr_addrs == NULL)
     goto out;
And all error, that may happen in guest_get_adapters_addresses will be 
held there.
>> +    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;
>> +            }
>> +
no need check if (!address_item), as Den had explained before.
>> +            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",
>>
>


[-- Attachment #2: Type: text/html, Size: 31214 bytes --]

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

[Olga Krishtal]

[-- Attachment #1: Type: text/plain, Size: 19689 bytes --]

On 29/05/15 19:46, Kirk Allan wrote:
>   >>>
>> On 28/05/15 23:54, Denis V. Lunev wrote:
>>> 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.
>>>
> I'll cleanup all the g_malloc testing for failures.
>
>>> this means that IP_ADAPTER_ADDRESSES could be returned from function
>> I think that check is still necessary, because we may suffer some other
>> problems:
>> *ERROR_ADDRESS_NOT_ASSOCIATED*
>> *ERROR_INVALID_PARAMETER*
>> and even *Other*, according to MSDN.
>>
>>   From my point of view, the prototype of function should be smth like this:
>> static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses()
>> {
>>
>> 	PIP_ADAPTER_ADDRESSES ip_add = NULL;
>> 	/* Memory allocation, etc */
>> 	......
>> 	if (GetAdaptersAddresses(....)) {
>> 		error_setg_win32(errp, GetLastError(), "Failed smth");
>> 		.....
>> 	}
>>
>>       return ip_add;
>>
>> }
> I'll make this change.  It does mean that errp will need to be passed in and then do something like
> ret = GetAdaptersAddresses(....);
> if (ret != ERROR_SUCCESS) {
> 	error_setg_win32(errp, ret, "Failed smth");
> 	.....
> }
> because GetAdpatersAddress is not documented to set last error.
Oh, yes. Sorry, but still I think that such prototype is better.
>> **
>>>> +}
>>>> +
>>>> +#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.
>>>
>> It is better to use #ifdef (smth), #else and #endif to separate win version.
> I'll redo the #ifdefs so that for the < 0x0600 case,
> guest_get_adapters_info and guest_ip_prefix will be together to better show what is happening for the XP and 2003 case.
>
>> Den is right, the first func will be compiled always, and the winxp
>> variation
>> will be compiled for winserver 2003, and winxp. Have you tested if for
>> 2003 server?
> I have tested on Windows 2003 with no issue.  The only way I have found to find the correct prefix on XP and 2003 is to use GetAdaptersInfo and do the matching.  The prefixes can be obtained from GetAdaptersAddresses but the order is not specified.  That's why I am just returning on ipv6 and calling GetAdaptersInfo to match addresses for ipv4.  Rather than do all this matching using GetAdaptersInfo, I could just return -1 as in the ipv6 case.  What do you recommend?
Actually, I have looked more attentive at  GetAdaptersInfo :

Minimum supported client

	Windows 2000 Professional [desktop apps only]

Minimum supported server

	Windows 2000 Server [desktop apps only]

and GetAdaptersAddresses:

Minimum supported client

	Windows XP [desktop apps only]

Minimum supported server

	Windows Server 2003 [desktop apps only]

May be we should drop the support of 2000 and do this functionality 
starting with XP. As the result we will not need this #ifdef thing.
>>>> +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
>> Same prototype as before.
> I'll clean up all the memory allocation checks.
>>>> +    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
> I'll clean up all the memory allocation checks.
>>>> +    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).as
>> px
> You're right, I'll remove the defined(ARCH_x86_64)
>> If you will use glib utility: nm_utils_inet4/6_ntop() you won't need
>> this separation.
> This would work, but I'm not seeing the header or lib to compile this in for Windows.  Maybe I'm missing some part of the environment?
I have searched rpm with nm-utils.h and the result is disappointing:   
NetworkManager-libnm-devel.XXX.rpm.
It would be too difficult task to take all this functionality to Windows.
Pls, look through this and say is it possible to support usage
of nm-utils functions in win32 realization. (I will do it too, but a bit 
latter).
However, we still can look at how it was implemented.
>>>> +    return (char *)InetNtop(af, cp, buf, len);
>>>> +#else
>>>> +    u_char *p;
> I'll change to use uint8_t.
>>>> +
>>>> +    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) {
> What do you recommend here?  I'm not finding a form of inet_ntop for all flavors of Window. The purpose of this code is to do the address compression.  For the XP and 2003 case, I could just simply produce the string of the full address.  Although not optimal, it would eliminate complexity and the need for the string manipulation.
>>>> +        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
> Tests have shown that this isn't an issue, but I can add the ().
>>>> +
>>>> +        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
>>>> +}
>>>> +
>> I still recommend to think over this function, here useful link
>> https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html
>> glib provides a lot of function to work with strings, and there you do
>> not need think
>> about memory allocations and etc. Pls, look.
>>
>>>> +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
> I'll do that.
>>>> +        if (guest_get_adapters_info(&adptr_info) != ERROR_SUCCESS) {
>>>> +            return prefix;
>>>> +        }
>> adptr_info = guest_get_adapters_info();
>> if(adptr_info == NULL )
>>       goto out;
>> If you will use the prototypes, I have written before it will be easier.
> I'll make the changes.
>>>> +        /* 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
> I'll do that.
>>>> +                }
>>>> +            }
>>>> +        }
>>>> +        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;
>>>> +
>> May be you should return  pointer to structure by
>> guest_get_adapters_addresses, and
>> make this function take void? As the result you won't need if (ret !=
>> ERROR_SUCCESS)
>> It will be smth like this:
>> adptr_addrs = guest_get_adapters_addresses();
>> if (adptr_addrs == NULL)
>>       goto out;
>> And all error, that may happen in guest_get_adapters_addresses will be
>> held there.
> I'll do that.
>>>> +    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
> I'll fix all the memory checks.
>>>> +            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
>>>
> I'll fix these
>>>> +            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;
>>>> +            }
>>>> +
>> no need check if (!address_item), as Den had explained before.
>>>> +            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
> I'll clean up all the memory allocation checks.
>>> 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",
>>>>


[-- Attachment #2: Type: text/html, Size: 28637 bytes --]

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

[Olga Krishtal]

[-- Attachment #1: Type: text/plain, Size: 20912 bytes --]

On 03/06/15 13:19, Olga Krishtal wrote:
> On 29/05/15 19:46, Kirk Allan wrote:
>>   >>>
>>> On 28/05/15 23:54, Denis V. Lunev wrote:
>>>> 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.
>>>>
>> I'll cleanup all the g_malloc testing for failures.
>>
>>>> this means that IP_ADAPTER_ADDRESSES could be returned from function
>>> I think that check is still necessary, because we may suffer some other
>>> problems:
>>> *ERROR_ADDRESS_NOT_ASSOCIATED*
>>> *ERROR_INVALID_PARAMETER*
>>> and even *Other*, according to MSDN.
>>>
>>>   From my point of view, the prototype of function should be smth 
>>> like this:
>>> static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses()
>>> {
>>>
>>>     PIP_ADAPTER_ADDRESSES ip_add = NULL;
>>>     /* Memory allocation, etc */
>>>     ......
>>>     if (GetAdaptersAddresses(....)) {
>>>         error_setg_win32(errp, GetLastError(), "Failed smth");
>>>         .....
>>>     }
>>>
>>>       return ip_add;
>>>
>>> }
>> I'll make this change.  It does mean that errp will need to be passed 
>> in and then do something like
>> ret = GetAdaptersAddresses(....);
>> if (ret != ERROR_SUCCESS) {
>>     error_setg_win32(errp, ret, "Failed smth");
>>     .....
>> }
>> because GetAdpatersAddress is not documented to set last error.
> Oh, yes. Sorry, but still I think that such prototype is better.
>>> **
>>>>> +}
>>>>> +
>>>>> +#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.
>>>>
>>> It is better to use #ifdef (smth), #else and #endif to separate win 
>>> version.
>> I'll redo the #ifdefs so that for the < 0x0600 case,
>> guest_get_adapters_info and guest_ip_prefix will be together to 
>> better show what is happening for the XP and 2003 case.
>>
>>> Den is right, the first func will be compiled always, and the winxp
>>> variation
>>> will be compiled for winserver 2003, and winxp. Have you tested if for
>>> 2003 server?
>> I have tested on Windows 2003 with no issue.  The only way I have 
>> found to find the correct prefix on XP and 2003 is to use 
>> GetAdaptersInfo and do the matching.  The prefixes can be obtained 
>> from GetAdaptersAddresses but the order is not specified.  That's why 
>> I am just returning on ipv6 and calling GetAdaptersInfo to match 
>> addresses for ipv4.  Rather than do all this matching using 
>> GetAdaptersInfo, I could just return -1 as in the ipv6 case.  What do 
>> you recommend?
> Actually, I have looked more attentive at  GetAdaptersInfo :
>
> Minimum supported client
>
>     Windows 2000 Professional [desktop apps only]
>
> Minimum supported server
>
>     Windows 2000 Server [desktop apps only]
>
> and GetAdaptersAddresses:
>
> Minimum supported client
>
>     Windows XP [desktop apps only]
>
> Minimum supported server
>
>     Windows Server 2003 [desktop apps only]
>
> May be we should drop the support of 2000 and do this functionality 
> starting with XP. As the result we will not need this #ifdef thing.
I have been looking through the list of function with *minimum supported 
client **Win 2003:*
OpenProcessToken, AdjustTokenPrivileges in qmp_guest_shutdown
SetFilePointerEx in qmp_guest_file_seek
BOOL WINAPI FlushFileBuffers in  qmp_guest_file_flush
BOOL WINAPI GetExitCodeProcess im qmp_guest_exec
Some IOCLS such as IOCTL_STORAGE_QUERY_PROPERTY
and etc.
As you can see at the moment support of Windows 2000 is very questionable.
I wonder if we could just drop it?


>>>>> +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
>>> Same prototype as before.
>> I'll clean up all the memory allocation checks.
>>>>> +    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
>> I'll clean up all the memory allocation checks.
>>>>> +    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).as 
>>>
>>> px
>> You're right, I'll remove the defined(ARCH_x86_64)
>>> If you will use glib utility: nm_utils_inet4/6_ntop() you won't need
>>> this separation.
>> This would work, but I'm not seeing the header or lib to compile this 
>> in for Windows.  Maybe I'm missing some part of the environment?
> I have searched rpm with nm-utils.h and the result is disappointing:   
> NetworkManager-libnm-devel.XXX.rpm.
> It would be too difficult task to take all this functionality to Windows.
> Pls, look through this and say is it possible to support usage
> of nm-utils functions in win32 realization. (I will do it too, but a 
> bit latter).
> However, we still can look at how it was implemented.
>>>>> +    return (char *)InetNtop(af, cp, buf, len);
>>>>> +#else
>>>>> +    u_char *p;
>> I'll change to use uint8_t.
>>>>> +
>>>>> +    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) {
>> What do you recommend here?  I'm not finding a form of inet_ntop for 
>> all flavors of Window. The purpose of this code is to do the address 
>> compression.  For the XP and 2003 case, I could just simply produce 
>> the string of the full address.  Although not optimal, it would 
>> eliminate complexity and the need for the string manipulation.
>>>>> +        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
>> Tests have shown that this isn't an issue, but I can add the ().
>>>>> +
>>>>> +        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
>>>>> +}
>>>>> +
>>> I still recommend to think over this function, here useful link
>>> https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html 
>>>
>>> glib provides a lot of function to work with strings, and there you do
>>> not need think
>>> about memory allocations and etc. Pls, look.
>>>
>>>>> +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
>> I'll do that.
>>>>> +        if (guest_get_adapters_info(&adptr_info) != ERROR_SUCCESS) {
>>>>> +            return prefix;
>>>>> +        }
>>> adptr_info = guest_get_adapters_info();
>>> if(adptr_info == NULL )
>>>       goto out;
>>> If you will use the prototypes, I have written before it will be 
>>> easier.
>> I'll make the changes.
>>>>> +        /* 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
>> I'll do that.
>>>>> +                }
>>>>> +            }
>>>>> +        }
>>>>> +        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;
>>>>> +
>>> May be you should return  pointer to structure by
>>> guest_get_adapters_addresses, and
>>> make this function take void? As the result you won't need if (ret !=
>>> ERROR_SUCCESS)
>>> It will be smth like this:
>>> adptr_addrs = guest_get_adapters_addresses();
>>> if (adptr_addrs == NULL)
>>>       goto out;
>>> And all error, that may happen in guest_get_adapters_addresses will be
>>> held there.
>> I'll do that.
>>>>> +    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
>> I'll fix all the memory checks.
>>>>> +            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
>>>>
>> I'll fix these
>>>>> +            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;
>>>>> +            }
>>>>> +
>>> no need check if (!address_item), as Den had explained before.
>>>>> +            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
>> I'll clean up all the memory allocation checks.
>>>> 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",
>>>>>
>
>


[-- Attachment #2: Type: text/html, Size: 41769 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On 05/28/2015 12:41 PM, 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()

s/Visa/Vista/

> and  OnLinkPrefixLength to get  IPv4 and IPv6 addresses and prefixes.

Why the  double  spacing?

> Setting –extra-cflags in the build configuration to

Again, Unicode mdash looks odd.

> ”- D_WIN32_WINNT-0x600 -DWINVER=0x600” or greater enables this functionality
> for those guests.  Setting –ectra-cflags is not required and if not used,

s/ectra/extra/

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

> +
> +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.
> +     */
> +    return (char *)InetNtop(af, cp, buf, len);
> +#else

Are you sure glib doesn't provide some sort of inet_ntop wrapper that
you could crib, instead of rolling your own?  And if you must roll your
own, do it as a separate patch from the rest of this work, possibly by
copying from glibc or other existing implementation (with proper credits
to the upstream source), rather than writing it from scratch.

> +    u_char *p;

u_char is not a standard typedef; uint8_t is more common.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Olga Krishtal]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

On 29/05/15 01:56, Eric Blake wrote:
> On 05/28/2015 12:41 PM, 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()
> s/Visa/Vista/
>
>> and  OnLinkPrefixLength to get  IPv4 and IPv6 addresses and prefixes.
> Why the  double  spacing?
>
>> Setting –extra-cflags in the build configuration to
> Again, Unicode mdash looks odd.
>
>> ”- D_WIN32_WINNT-0x600 -DWINVER=0x600” or greater enables this functionality
>> for those guests.  Setting –ectra-cflags is not required and if not used,
> s/ectra/extra/
>
>> 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(-)
>> +
>> +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.
>> +     */
>> +    return (char *)InetNtop(af, cp, buf, len);
>> +#else
> Are you sure glib doesn't provide some sort of inet_ntop wrapper that
> you could crib, instead of rolling your own?  And if you must roll your
> own, do it as a separate patch from the rest of this work, possibly by
> copying from glibc or other existing implementation (with proper credits
> to the upstream source), rather than writing it from scratch.
Agree. Moreover, there is separate functions for inet4 and inet6.
Pls, look here https://developer.gnome.org/libnm/stable/libnm-nm-utils.html
Here you can find nm_utils_inet4/6_ntop() description.

>> +    u_char *p;
> u_char is not a standard typedef; uint8_t is more common.
>


[-- Attachment #2: Type: text/html, Size: 3420 bytes --]

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

[Olga Krishtal]

On 29/05/15 12:59, Olga Krishtal wrote:
> On 29/05/15 01:56, Eric Blake wrote:
>> On 05/28/2015 12:41 PM, 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()
>> s/Visa/Vista/
>>
>>> and  OnLinkPrefixLength to get  IPv4 and IPv6 addresses and prefixes.
>> Why the  double  spacing?
>>
>>> Setting –extra-cflags in the build configuration to
>> Again, Unicode mdash looks odd.
>>
>>> ”- D_WIN32_WINNT-0x600 -DWINVER=0x600” or greater enables this 
>>> functionality
>>> for those guests.  Setting –ectra-cflags is not required and if not 
>>> used,
>> s/ectra/extra/
>>
>>> 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(-)
>>> +
>>> +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.
>>> +     */
>>> +    return (char *)InetNtop(af, cp, buf, len);
>>> +#else
>> Are you sure glib doesn't provide some sort of inet_ntop wrapper that
>> you could crib, instead of rolling your own?  And if you must roll your
>> own, do it as a separate patch from the rest of this work, possibly by
>> copying from glibc or other existing implementation (with proper credits
>> to the upstream source), rather than writing it from scratch.
> Agree. Moreover, there is separate functions for inet4 and inet6.
> Pls, look here 
> https://developer.gnome.org/libnm/stable/libnm-nm-utils.html
> Here you can find nm_utils_inet4/6_ntop() description.
>
>>> +    u_char *p;
>> u_char is not a standard typedef; uint8_t is more common.
>>
>
>
Eric, I have looked attentively at glib utils for nm_utils_inet4/6_ntop().
Header to use this function can be found in 
NetworkManager-libnm-devel.XXX.rpm.
It would be difficult to support all its functionality. So, the only 
way, as I see it, to make
out own realization (may be just look at how it was implemented in 
nm-utils.h )