All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/2] qga: implement qmp_guest_network_get_interfaces for win32
@ 2015-04-01 15:39 Kirk Allan
  2015-04-01 15:39 ` [Qemu-devel] [PATCH v2 1/2] qga: add additional win32 cflags and libraries Kirk Allan
  2015-04-01 15:39 ` [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces Kirk Allan
  0 siblings, 2 replies; 5+ messages in thread
From: Kirk Allan @ 2015-04-01 15:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: sw, mdroth

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 | 319 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 324 insertions(+), 4 deletions(-)

-- 
1.8.5.6

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Qemu-devel] [PATCH v2 1/2] qga: add additional win32 cflags and libraries
  2015-04-01 15:39 [Qemu-devel] [PATCH v2 0/2] qga: implement qmp_guest_network_get_interfaces for win32 Kirk Allan
@ 2015-04-01 15:39 ` Kirk Allan
  2015-04-01 15:39 ` [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces Kirk Allan
  1 sibling, 0 replies; 5+ messages in thread
From: Kirk Allan @ 2015-04-01 15:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: sw, mdroth, 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 09c9225..3d78bc4 100755
--- a/configure
+++ b/configure
@@ -700,7 +700,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"
@@ -718,7 +723,7 @@ EOF
   sysconfdir="\${prefix}"
   local_statedir=
   confsuffix=""
-  libs_qga="-lws2_32 -lwinmm -lpowrprof $libs_qga"
+  libs_qga="-lws2_32 -lwinmm -lpowrprof -liphlpapi $libs_qga"
 fi
 
 werror=""
-- 
1.8.5.6

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces
  2015-04-01 15:39 [Qemu-devel] [PATCH v2 0/2] qga: implement qmp_guest_network_get_interfaces for win32 Kirk Allan
  2015-04-01 15:39 ` [Qemu-devel] [PATCH v2 1/2] qga: add additional win32 cflags and libraries Kirk Allan
@ 2015-04-01 15:39 ` Kirk Allan
  2015-05-20 18:43   ` Olga Krishtal
  1 sibling, 1 reply; 5+ messages in thread
From: Kirk Allan @ 2015-04-01 15:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: sw, mdroth, 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 GetApaptersInfo.  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 | 319 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 317 insertions(+), 2 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3ef0549..635d3ca 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -16,11 +16,17 @@
 #include <powrprof.h>
 #include <stdio.h>
 #include <string.h>
+#include <winsock2.h>
+#include <ws2tcpip.h>
+#include <ws2ipdef.h>
+#include <iptypes.h>
+#include <iphlpapi.h>
 #include "qga/guest-agent-core.h"
 #include "qga/vss-win32.h"
 #include "qga-qmp-commands.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/queue.h"
+#include "qemu/host-utils.h"
 
 #ifndef SHTDN_REASON_FLAG_PLANNED
 #define SHTDN_REASON_FLAG_PLANNED 0x80000000
@@ -589,9 +595,318 @@ void qmp_guest_suspend_hybrid(Error **errp)
     error_set(errp, QERR_UNSUPPORTED);
 }
 
+#define WORKING_BUFFER_SIZE 15000
+#define MAX_ALLOC_TRIES 2
+
+static DWORD guest_get_adapter_addresses(IP_ADAPTER_ADDRESSES **adptr_addrs)
+{
+    ULONG out_buf_len = 0;
+    int alloc_tries = 0;
+    DWORD ret = ERROR_SUCCESS;
+
+    /* Allocate a 15 KB buffer to start with.  If not enough, out_buf_len
+     * will have the amount needed after the call to GetAdaptersAddresses.
+     */
+    out_buf_len = WORKING_BUFFER_SIZE;
+
+    do {
+        *adptr_addrs = (IP_ADAPTER_ADDRESSES *) g_malloc(out_buf_len);
+        if (*adptr_addrs == NULL) {
+            return ERROR_NOT_ENOUGH_MEMORY;
+        }
+
+        ret = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
+            NULL, *adptr_addrs, &out_buf_len);
+
+        if (ret == ERROR_BUFFER_OVERFLOW) {
+            g_free(*adptr_addrs);
+            *adptr_addrs = NULL;
+            alloc_tries++;
+        }
+
+    } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries < MAX_ALLOC_TRIES));
+
+    if (ret != ERROR_SUCCESS) {
+        if (*adptr_addrs) {
+            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 out_buf_len = 0;
+    int alloc_tries = 0;
+    DWORD ret = ERROR_SUCCESS;
+
+    out_buf_len = sizeof(IP_ADAPTER_INFO);
+    do {
+        *adptr_info = (IP_ADAPTER_INFO *)g_malloc(out_buf_len);
+        if (*adptr_info == NULL) {
+            return ERROR_NOT_ENOUGH_MEMORY;
+        }
+
+        ret = GetAdaptersInfo(*adptr_info, &out_buf_len);
+
+        if (ret == ERROR_BUFFER_OVERFLOW) {
+            g_free(*adptr_info);
+            *adptr_info = NULL;
+            alloc_tries++;
+        }
+
+    } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries < MAX_ALLOC_TRIES));
+
+    if (ret != ERROR_SUCCESS) {
+        if (*adptr_info) {
+            g_free(*adptr_info);
+            *adptr_info = NULL;
+        }
+    }
+    return ret;
+}
+#endif
+
+static char *guest_wcstombs_dup(WCHAR *wstr)
+{
+    char *str;
+    int i;
+
+    i = wcslen(wstr) + 1;
+    str = g_malloc(i);
+    if (str) {
+        wcstombs(str, wstr, i);
+    }
+    return str;
+}
+
+static char *guest_inet_ntop(int af, void *cp, char *buf, socklen_t len)
+{
+#if (_WIN32_WINNT >= 0x0600) && defined(ARCH_x86_64)
+    /* If built for 64 bit Windows Vista/2008 or newer, inet_ntop() is
+     * available for use.  Otherwise, do our best to derive it.
+     */
+    return (char *)inet_ntop(af, cp, buf, len);
+#else
+    u_char *p;
+
+    p = (u_char *)cp;
+    if (af == AF_INET) {
+        snprintf(buf, len, "%u.%u.%u.%u", p[0], p[1], p[2], p[3]);
+    } else if (af == AF_INET6) {
+        int i, compressed_zeros, added_to_buf;
+        char t[sizeof "::ffff"];
+
+        buf[0] = '\0';
+        compressed_zeros = 0;
+        added_to_buf = 0;
+        for (i = 0; i < 16; i += 2) {
+            if (p[i] != 0 || p[i + 1] != 0) {
+                if (compressed_zeros) {
+                    if (len > sizeof "::") {
+                        strcat(buf, "::");
+                        len -= sizeof "::" - 1;
+                    }
+                    added_to_buf++;
+                } else {
+                    if (added_to_buf) {
+                        if (len > 1) {
+                            strcat(buf, ":");
+                            len--;
+                        }
+                    }
+                    added_to_buf++;
+                }
+
+                /* Take into account leading zeros. */
+                if (p[i]) {
+                    len -= snprintf(t, sizeof(t), "%x%02x", p[i], p[i+1]);
+                } else {
+                    len -= snprintf(t, sizeof(t), "%x", p[i+1]);
+                }
+
+                /* Ensure there's enough room for the NULL. */
+                if (len > 0) {
+                    strcat(buf, t);
+                }
+                compressed_zeros = 0;
+            } else {
+                compressed_zeros++;
+            }
+        }
+        if (compressed_zeros && !added_to_buf) {
+            /* The address was all zeros. */
+            strcat(buf, "::");
+        }
+    }
+    return buf;
+#endif
+}
+
+static int64_t guest_ip_prefix(IP_ADAPTER_UNICAST_ADDRESS *ip_addr)
+{
+#if (_WIN32_WINNT >= 0x0600)
+    /* If built for Windows Vista/2008 or newer, use the OnLinkPrefixLength
+     * field to obtain the prefix.  Otherwise, do our best to figure it out.
+     */
+    return ip_addr->OnLinkPrefixLength;
+#else
+    int64_t prefix = -1; /* Use for AF_INET6 and unknown/undetermined values. */
+
+    if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
+        IP_ADAPTER_INFO *adptr_info, *info;
+        IP_ADDR_STRING *ip;
+        struct in_addr *p;
+
+        if (guest_get_adapters_info(&adptr_info) != ERROR_SUCCESS) {
+            return prefix;
+        }
+
+        /* Match up the passed in ip_addr with one found in adaptr_info.
+         * The matching one in adptr_info will have the netmask.
+         */
+        p = &((struct sockaddr_in *)ip_addr->Address.lpSockaddr)->sin_addr;
+        for (info = adptr_info; info && prefix == -1; info = info->Next) {
+            for (ip = &info->IpAddressList; ip; ip = ip->Next) {
+                if (p->S_un.S_addr == inet_addr(ip->IpAddress.String)) {
+                    prefix = ctpop32(inet_addr(ip->IpMask.String));
+                    break;
+                }
+            }
+        }
+        g_free(adptr_info);
+    }
+    return prefix;
+#endif
+}
+
 GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
 {
-    error_set(errp, QERR_UNSUPPORTED);
+    IP_ADAPTER_ADDRESSES *adptr_addrs, *addr;
+    GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
+    GuestIpAddressList *head_addr, *cur_addr;
+    DWORD ret;
+
+    ret = guest_get_adapter_addresses(&adptr_addrs);
+    if (ret != ERROR_SUCCESS) {
+        error_setg(errp, "failed to get adapter addresses %lu", ret);
+        return NULL;
+    }
+
+    for (addr = adptr_addrs; addr; addr = addr->Next) {
+        GuestNetworkInterfaceList *info;
+        GuestIpAddressList *address_item = NULL;
+        char addr4[INET_ADDRSTRLEN];
+        char addr6[INET6_ADDRSTRLEN];
+        unsigned char *mac_addr;
+        void *p;
+        IP_ADAPTER_UNICAST_ADDRESS *ip_addr = NULL;
+
+        info = g_malloc0(sizeof(*info));
+        if (!info) {
+            error_setg(errp, "failed to alloc a network interface list");
+            goto error;
+        }
+
+        if (!cur_item) {
+            head = cur_item = info;
+        } else {
+            cur_item->next = info;
+            cur_item = info;
+        }
+
+        info->value = g_malloc0(sizeof(*info->value));
+        if (!info->value) {
+            error_setg(errp, "failed to alloc a network interface");
+            goto error;
+        }
+        info->value->name = guest_wcstombs_dup(addr->FriendlyName);
+
+        if (addr->PhysicalAddressLength) {
+            mac_addr = addr->PhysicalAddress;
+
+            info->value->hardware_address =
+                g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
+                    (int) mac_addr[0], (int) mac_addr[1],
+                    (int) mac_addr[2], (int) mac_addr[3],
+                    (int) mac_addr[4], (int) mac_addr[5]);
+
+            info->value->has_hardware_address = true;
+        }
+
+        head_addr = NULL;
+        cur_addr = NULL;
+        for (ip_addr = addr->FirstUnicastAddress;
+                ip_addr;
+                ip_addr = ip_addr->Next) {
+            address_item = g_malloc0(sizeof(*address_item));
+            if (!address_item) {
+                error_setg(errp, "failed to alloc an Ip address list");
+                goto error;
+            }
+
+            if (!cur_addr) {
+                head_addr = cur_addr = address_item;
+            } else {
+                cur_addr->next = address_item;
+                cur_addr = address_item;
+            }
+
+            address_item->value = g_malloc0(sizeof(*address_item->value));
+            if (!address_item->value) {
+                error_setg(errp, "failed to alloc an Ip address");
+                goto error;
+            }
+
+            if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
+                p = &((struct sockaddr_in *)
+                    ip_addr->Address.lpSockaddr)->sin_addr;
+
+                if (!guest_inet_ntop(AF_INET, p, addr4, sizeof(addr4))) {
+                    error_setg(errp,
+                               "failed address presentation form conversion");
+                    goto error;
+                }
+
+                address_item->value->ip_address = g_strdup(addr4);
+                address_item->value->ip_address_type =
+                    GUEST_IP_ADDRESS_TYPE_IPV4;
+            } else if (ip_addr->Address.lpSockaddr->sa_family == AF_INET6) {
+                p = &((struct sockaddr_in6 *)
+                    ip_addr->Address.lpSockaddr)->sin6_addr;
+
+                if (!guest_inet_ntop(AF_INET6, p, addr6, sizeof(addr6))) {
+                    error_setg(errp,
+                               "failed address presentation form conversion");
+                    goto error;
+                }
+
+                address_item->value->ip_address = g_strdup(addr6);
+                address_item->value->ip_address_type =
+                    GUEST_IP_ADDRESS_TYPE_IPV6;
+            }
+            address_item->value->prefix = guest_ip_prefix(ip_addr);
+        }
+        if (head_addr) {
+            info->value->has_ip_addresses = true;
+            info->value->ip_addresses = head_addr;
+        }
+    }
+
+    if (adptr_addrs) {
+        g_free(adptr_addrs);
+    }
+    return head;
+
+error:
+    if (adptr_addrs) {
+        g_free(adptr_addrs);
+    }
+    if (head) {
+        qapi_free_GuestNetworkInterfaceList(head);
+    }
     return NULL;
 }
 
@@ -707,7 +1022,7 @@ GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
 GList *ga_command_blacklist_init(GList *blacklist)
 {
     const char *list_unsupported[] = {
-        "guest-suspend-hybrid", "guest-network-get-interfaces",
+        "guest-suspend-hybrid",
         "guest-get-vcpus", "guest-set-vcpus",
         "guest-set-user-password",
         "guest-get-memory-blocks", "guest-set-memory-blocks",
-- 
1.8.5.6

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces
  2015-04-01 15:39 ` [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces Kirk Allan
@ 2015-05-20 18:43   ` Olga Krishtal
  2015-05-28 14:33     ` Kirk Allan
  0 siblings, 1 reply; 5+ messages in thread
From: Olga Krishtal @ 2015-05-20 18:43 UTC (permalink / raw)
  To: Kirk Allan, qemu-devel; +Cc: sw, mdroth

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces
  2015-05-20 18:43   ` Olga Krishtal
@ 2015-05-28 14:33     ` Kirk Allan
  0 siblings, 0 replies; 5+ messages in thread
From: Kirk Allan @ 2015-05-28 14:33 UTC (permalink / raw)
  To: qemu-devel, Olga Krishtal; +Cc: sw, mdroth

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

Kirk

 >>>
> On 01/04/15 18:39, Kirk Allan wrote:
>> By default, IP addresses and prefixes will be derived from information
>> obtained by various calls and structures.  IPv4 prefixes can be found
>> by matching the address to those returned by GetApaptersInfo.  IPv6
>> prefixes can not be matched this way due to the unpredictable order of
>> entries.
> GetAdaptersInfo.

I'll fix the typo.

>> In Windows Visa/2008 guests and newer, it is possible to use inet_ntop()
>> and  OnLinkPrefixLength to get  IPv4 and IPv6 addresses and prefixes.
>> Setting *extra-cflags in the build configuration to
>> *- D_WIN32_WINNT-0x600 -DWINVER=0x600* or greater enables this functionality
>> for those guests.  Setting *ectra-cflags is not required and if not used,
>> the default approach will be taken.
>>
>> Signed-off-by: Kirk Allan <kallan@suse.com>
>> ---
>>   qga/commands-win32.c | 319 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 317 insertions(+), 2 deletions(-)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 3ef0549..635d3ca 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -16,11 +16,17 @@
>>   #include <powrprof.h>
>>   #include <stdio.h>
>>   #include <string.h>
>> +#include <winsock2.h>
>> +#include <ws2tcpip.h>
>> +#include <ws2ipdef.h>
>> +#include <iptypes.h>
>> +#include <iphlpapi.h>
>>   #include "qga/guest-agent-core.h"
>>   #include "qga/vss-win32.h"
>>   #include "qga-qmp-commands.h"
>>   #include "qapi/qmp/qerror.h"
>>   #include "qemu/queue.h"
>> +#include "qemu/host-utils.h"
>>   
>>   #ifndef SHTDN_REASON_FLAG_PLANNED
>>   #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>> @@ -589,9 +595,318 @@ void qmp_guest_suspend_hybrid(Error **errp)
>>       error_set(errp, QERR_UNSUPPORTED);
>>   }
>>   
>> +#define WORKING_BUFFER_SIZE 15000
>> +#define MAX_ALLOC_TRIES 2
>> +
>> +static DWORD guest_get_adapter_addresses(IP_ADAPTER_ADDRESSES 
> **adptr_addrs)
>> +{
>> +    ULONG out_buf_len = 0;
>> +    int alloc_tries = 0;
>> +    DWORD ret = ERROR_SUCCESS;
>> +
>> +    /* Allocate a 15 KB buffer to start with.  If not enough, out_buf_len
>> +     * will have the amount needed after the call to GetAdaptersAddresses.
>> +     */
> may be we should not allocate memory in the beginning and just make a 
> call with just pointer,
> and get necessary size.

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

>> +    out_buf_len = WORKING_BUFFER_SIZE;
>> +
>> +    do {
>> +        *adptr_addrs = (IP_ADAPTER_ADDRESSES *) g_malloc(out_buf_len);
>> +        if (*adptr_addrs == NULL) {
>> +            return ERROR_NOT_ENOUGH_MEMORY;
> I think we should use error handling with win32 macro error_setg_win32( 
> errno, GetLastError()..

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

>> +        }
>> +
>> +        ret = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
>> +            NULL, *adptr_addrs, &out_buf_len);
>> +
>> +        if (ret == ERROR_BUFFER_OVERFLOW) {
>> +            g_free(*adptr_addrs);
>> +            *adptr_addrs = NULL;
>> +            alloc_tries++;
>> +        }
>> +
>> +    } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries < 
> MAX_ALLOC_TRIES));
> Why we do it twice?
>> +    if (ret != ERROR_SUCCESS) {
>> +        if (*adptr_addrs) {
>> +            g_free(*adptr_addrs);
>> +            *adptr_addrs = NULL;
>> +        }
>> +    }
> In this case we always have ERROR_SUCCESS.
>> +    return ret;
>> +}
>> +
> The comments for this function is the same. Moreover, they look very 
> similar,
> can we merge them and have #if (_WIN32_WINNT < 0x0600) inside the 
> function body?

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

>> +#if (_WIN32_WINNT < 0x0600)
>> +static DWORD guest_get_adapters_info(IP_ADAPTER_INFO **adptr_info)
>> +{
>> +    ULONG out_buf_len = 0;
>> +    int alloc_tries = 0;
>> +    DWORD ret = ERROR_SUCCESS;
>> +
>> +    out_buf_len = sizeof(IP_ADAPTER_INFO);
>> +    do {
>> +        *adptr_info = (IP_ADAPTER_INFO *)g_malloc(out_buf_len);
>> +        if (*adptr_info == NULL) {
>> +            return ERROR_NOT_ENOUGH_MEMORY;
>> +        }
>> +
>> +        ret = GetAdaptersInfo(*adptr_info, &out_buf_len);
>> +
>> +        if (ret == ERROR_BUFFER_OVERFLOW) {
>> +            g_free(*adptr_info);
>> +            *adptr_info = NULL;
>> +            alloc_tries++;
>> +        }
>> +
>> +    } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries < 
> MAX_ALLOC_TRIES));
>> +
>> +    if (ret != ERROR_SUCCESS) {
>> +        if (*adptr_info) {
>> +            g_free(*adptr_info);
>> +            *adptr_info = NULL;
>> +        }
>> +    }
>> +    return ret;
>> +}
>> +#endif
>> +static char *guest_wcstombs_dup(WCHAR *wstr)
>> +{
>> +    char *str;
>> +    int i;
>> +
>> +    i = wcslen(wstr) + 1;
>> +    str = g_malloc(i);
>> +    if (str) {
> This is Windows-specific part of qemu-ga, so we should win32 API use 
> WideCharToMultiByte.
> I am not sure that wcstombs uses Unicode UTF-16 and afaik it is deprecated.

Will switch to WideCharToMultiByte.

>> +        wcstombs(str, wstr, i);
>> +    }
>> +    return str;
>> +}
>> +
>> +static char *guest_inet_ntop(int af, void *cp, char *buf, socklen_t len)
>> +{
>> +#if (_WIN32_WINNT >= 0x0600) && defined(ARCH_x86_64)
>> +    /* If built for 64 bit Windows Vista/2008 or newer, inet_ntop() is
>> +     * available for use.  Otherwise, do our best to derive it.
>> +     */
>> +    return (char *)inet_ntop(af, cp, buf, len);
>> +#else
>> +    u_char *p;
>> +
>> +    p = (u_char *)cp;
>> +    if (af == AF_INET) {
> May be we should do some struct IP_addr that will hold this information 
> in appropriate format.

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

>> +        snprintf(buf, len, "%u.%u.%u.%u", p[0], p[1], p[2], p[3]);
>> +    } else if (af == AF_INET6) {
>> +        int i, compressed_zeros, added_to_buf;
>> +        char t[sizeof "::ffff"];
>> +
>> +        buf[0] = '\0';
>> +        compressed_zeros = 0;
>> +        added_to_buf = 0;
>> +        for (i = 0; i < 16; i += 2) {
>> +            if (p[i] != 0 || p[i + 1] != 0) {
>> +                if (compressed_zeros) {
>> +                    if (len > sizeof "::") {
>> +                        strcat(buf, "::");
>> +                        len -= sizeof "::" - 1;
>> +                    }
>> +                    added_to_buf++;
>> +                } else {
>> +                    if (added_to_buf) {
>> +                        if (len > 1) {
>> +                            strcat(buf, ":");
>> +                            len--;
>> +                        }
>> +                    }
>> +                    added_to_buf++;
>> +                }
>> +
>> +                /* Take into account leading zeros. */
>> +                if (p[i]) {
>> +                    len -= snprintf(t, sizeof(t), "%x%02x", p[i], p[i+1]);
>> +                } else {
>> +                    len -= snprintf(t, sizeof(t), "%x", p[i+1]);
>> +                }
>> +
>> +                /* Ensure there's enough room for the NULL. */
>> +                if (len > 0) {
>> +                    strcat(buf, t);
>> +                }
>> +                compressed_zeros = 0;
>> +            } else {
>> +                compressed_zeros++;
>> +            }
>> +        }
>> +        if (compressed_zeros && !added_to_buf) {
>> +            /* The address was all zeros. */
>> +            strcat(buf, "::");
>> +        }
>> +    }
>> +    return buf;
>> +#endif
>> +}
>> +
>> +static int64_t guest_ip_prefix(IP_ADAPTER_UNICAST_ADDRESS *ip_addr)
>> +{
>> +#if (_WIN32_WINNT >= 0x0600
)
>> +    /* If built for Windows Vista/2008 or newer, use the OnLinkPrefixLength
>> +     * field to obtain the prefix.  Otherwise, do our best to figure it 
> out.
>> +     */
>> +    return ip_addr->OnLinkPrefixLength;
>> +#else
>> +    int64_t prefix = -1; /* Use for AF_INET6 and unknown/undetermined 
> values. */
>> +
>> +    if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
>> +        IP_ADAPTER_INFO *adptr_info, *info;
>> +        IP_ADDR_STRING *ip;
>> +        struct in_addr *p;
>> +
>> +        if (guest_get_adapters_info(&adptr_info) != ERROR_SUCCESS) {
>> +            return prefix;
>> +        }
>> +
>> +        /* Match up the passed in ip_addr with one found in adaptr_info.
>> +         * The matching one in adptr_info will have the netmask.
>> +         */
>> +        p = &((struct sockaddr_in *)ip_addr->Address.lpSockaddr)->sin_addr;
>> +        for (info = adptr_info; info && prefix == -1; info = info->Next) {
>> +            for (ip = &info->IpAddressList; ip; ip = ip->Next) {
>> +                if (p->S_un.S_addr == inet_addr(ip->IpAddress.String)) {
>> +                    prefix = ctpop32(inet_addr(ip->IpMask.String));
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +        g_free(adptr_info);
>> +    }
>> +    return prefix;
>> +#endif
>> +}
>> +
>>   GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>>   {
>> -    error_set(errp, QERR_UNSUPPORTED);
>> +    IP_ADAPTER_ADDRESSES *adptr_addrs, *addr;
>> +    GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
>> +    GuestIpAddressList *head_addr, *cur_addr;
>> +    DWORD ret;
>> +
>> +    ret = guest_get_adapter_addresses(&adptr_addrs);
>> +    if (ret != ERROR_SUCCESS) {
>> +        error_setg(errp, "failed to get adapter addresses %lu", ret);
>> +        return NULL;
>> +    }
>> +
>> +    for (addr = adptr_addrs; addr; addr = addr->Next) {
>> +        GuestNetworkInterfaceList *info;
>> +        GuestIpAddressList *address_item = NULL;
>> +        char addr4[INET_ADDRSTRLEN];
>> +        char addr6[INET6_ADDRSTRLEN];
>> +        unsigned char *mac_addr;
>> +        void *p;
> Variables should not be declared in function body. I think better way is 
> to put them in the beginning.

I'll move the variable to the beginning.

>> +        IP_ADAPTER_UNICAST_ADDRESS *ip_addr = NULL;
>> +
>> +        info = g_malloc0(sizeof(*info));
>> +        if (!info) {
>> +            error_setg(errp, "failed to alloc a network interface list");
>> +            goto error;
>> +        }
>> +
>> +        if (!cur_item) {
>> +            head = cur_item = info;
>> +        } else {
>> +            cur_item->next = info;
>> +            cur_item = info;
>> +        }
>> +
>> +        info->value = g_malloc0(sizeof(*info->value));
>> +        if (!info->value) {
>> +            error_setg(errp, "failed to alloc a network interface");
>> +            goto error;
>> +        }
>> +        info->value->name = guest_wcstombs_dup(addr->FriendlyName);
>> +
>> +        if (addr->PhysicalAddressLength) {
>> +            mac_addr = addr->PhysicalAddress;
>> +
>> +            info->value->hardware_address =
>> +                g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
>> +                    (int) mac_addr[0], (int) mac_addr[1],
>> +                    (int) mac_addr[2], (int) mac_addr[3],
>> +                    (int) mac_addr[4], (int) mac_addr[5]);
>> +
>> +            info->value->has_hardware_address = true;
>> +        }
>> +
>> +        head_addr = NULL;
>> +        cur_addr = NULL;
>> +        for (ip_addr = addr->FirstUnicastAddress;
>> +                ip_addr;
>> +                ip_addr = ip_addr->Next) {
>> +            address_item = g_malloc0(sizeof(*address_item));
>> +            if (!address_item) {
>> +                error_setg(errp, "failed to alloc an Ip address list");
>> +                goto error;
>> +            }
>> +
>> +            if (!cur_addr) {
>> +
            head_addr = cur_addr = address_item;
>> +            } else {
>> +                cur_addr->next = address_item;
>> +                cur_addr = address_item;
>> +            }
>> +
>> +            address_item->value = g_malloc0(sizeof(*address_item->value));
>> +            if (!address_item->value) {
>> +                error_setg(errp, "failed to alloc an Ip address");
>> +                goto error;
>> +            }
>> +
>> +            if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
>> +                p = &((struct sockaddr_in *)
>> +                    ip_addr->Address.lpSockaddr)->sin_addr;
>> +
>> +                if (!guest_inet_ntop(AF_INET, p, addr4, sizeof(addr4))) {
>> +            		error_setg(errp,
> win32 error macro

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

>> +                               "failed address presentation form 
> conversion");
>> +                    goto error;
>> +                }
>> +
>> +                address_item->value->ip_address = g_strdup(addr4);
>> +                address_item->value->ip_address_type =
>> +                    GUEST_IP_ADDRESS_TYPE_IPV4;
>> +            } else if (ip_addr->Address.lpSockaddr->sa_family == AF_INET6) {
>> +                p = &((struct sockaddr_in6 *)
>> +                    ip_addr->Address.lpSockaddr)->sin6_addr;
>> +
>> +                if (!guest_inet_ntop(AF_INET6, p, addr6, sizeof(addr6))) {
>> +                    error_setg(errp,
>> +                               "failed address presentation form 
> conversion");
> win32 error macro

Same as above.

>>                      goto error;
>> +                }
>> +
>> +                address_item->value->ip_address = g_strdup(addr6);
>> +                address_item->value->ip_address_type =
>> +                    GUEST_IP_ADDRESS_TYPE_IPV6;
>> +            }
>> +            address_item->value->prefix = guest_ip_prefix(ip_addr);
>> +        }
>> +        if (head_addr) {
>> +            info->value->has_ip_addresses = true;
>> +            info->value->ip_addresses = head_addr;
>> +        }
>> +    }
>> +
>> +    if (adptr_addrs) {
>> +        g_free(adptr_addrs);
>> +    }
>> +    return head;
>> +
>> +error:
>> +    if (adptr_addrs) {
>> +        g_free(adptr_addrs);
>> +    }
>> +    if (head) {
>> +        qapi_free_GuestNetworkInterfaceList(head);
>> +    }
>>       return NULL;
>>   }
>>   
>> @@ -707,7 +1022,7 @@ GuestMemoryBlockInfo 
> *qmp_guest_get_memory_block_info(Error **errp)
>>   GList *ga_command_blacklist_init(GList *blacklist)
>>   {
>>       const char *list_unsupported[] = {
>> -        "guest-suspend-hybrid", "guest-network-get-interfaces",
>> +        "guest-suspend-hybrid",
>>           "guest-get-vcpus", "guest-set-vcpus",
>>           "guest-set-user-password",
>>           "guest-get-memory-blocks", "guest-set-memory-blocks",
> I think 2 last functions  GuestNetworkInterfaceList 
> *qmp_guest_network_get_interfaces(Error **errp) and  static char 
> *guest_inet_ntop(int af, void *cp, char *buf, socklen_t len)should be 
> redesigned and refactored very attentively. Secondly you should decide 
> what kind of functions to use Win32 API or POSIX API.
> It is bed idea to mix them.

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

> Pls, pay attention to to error handling.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-05-28 14:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-01 15:39 [Qemu-devel] [PATCH v2 0/2] qga: implement qmp_guest_network_get_interfaces for win32 Kirk Allan
2015-04-01 15:39 ` [Qemu-devel] [PATCH v2 1/2] qga: add additional win32 cflags and libraries Kirk Allan
2015-04-01 15:39 ` [Qemu-devel] [PATCH v2 2/2] qga: win32 implementation of qmp_guest_network_get_interfaces Kirk Allan
2015-05-20 18:43   ` Olga Krishtal
2015-05-28 14:33     ` Kirk Allan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.