* [Qemu-devel] [PATCH v2] qemu-ga: added windows support for 'guest-network-get-interfaces'
@ 2014-09-29 18:22 Kenth Andersson
2014-10-01 17:45 ` Michael Roth
0 siblings, 1 reply; 3+ messages in thread
From: Kenth Andersson @ 2014-09-29 18:22 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU, Michael Roth
Implementation of guest-network-get-interfaces for windows
Signed-off-by: Kenth Andersson <kenth@eastmark.net>
---
configure | 2 +-
qga/commands-win32.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 244 insertions(+), 5 deletions(-)
diff --git a/configure b/configure
index 681abfc..7c6c60c 100755
--- a/configure
+++ b/configure
@@ -717,7 +717,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=""
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3bcbeae..fb6fdba 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -14,6 +14,9 @@
#include <glib.h>
#include <wtypes.h>
#include <powrprof.h>
+#include <winsock2.h>
+#include <iphlpapi.h>
+#include <ws2tcpip.h>
#include "qga/guest-agent-core.h"
#include "qga/vss-win32.h"
#include "qga-qmp-commands.h"
@@ -29,6 +32,9 @@
(365 * (1970 - 1601) + \
(1970 - 1601) / 4 - 3))
+/* Defines the max length of an IPv6 address in human readable format + pad */
+#define MAX_IPv6_LEN 50
+
static void acquire_privilege(const char *name, Error **errp)
{
HANDLE token = NULL;
@@ -359,10 +365,244 @@ void qmp_guest_suspend_hybrid(Error **errp)
error_set(errp, QERR_UNSUPPORTED);
}
+static int get_prefix_length(PIP_ADAPTER_PREFIX prefix,
+ SOCKET_ADDRESS *sockaddr, int socklen)
+{
+ PIP_ADAPTER_PREFIX p;
+ struct sockaddr *addr = sockaddr->lpSockaddr;
+
+ for (p = prefix; p; p = p->Next) {
+ /* Compare the ip-adderss address family with the prefix
+ * address family */
+ if (addr->sa_family == p->Address.lpSockaddr->sa_family) {
+ int num_bytes = p->PrefixLength / 8;
+ unsigned char *src = 0;
+ unsigned char *dst = 0;
+ int remaining_bits;
+
+ if (addr->sa_family == AF_INET) {
+ struct sockaddr_in *sin = (struct sockaddr_in *)addr;
+ src = (unsigned char *)&(sin->sin_addr.s_addr);
+ } else if (addr->sa_family == AF_INET6) {
+ struct sockaddr_in6 *sin = (struct sockaddr_in6 *)addr;
+ src = (unsigned char *)sin->sin6_addr.s6_addr;
+ }
+
+ if (p->Address.lpSockaddr->sa_family == AF_INET) {
+ struct sockaddr_in *sin =
+ (struct sockaddr_in *)p->Address.lpSockaddr;
+ dst = (unsigned char *)&(sin->sin_addr.s_addr);
+ } else if (p->Address.lpSockaddr->sa_family == AF_INET6) {
+ struct sockaddr_in6 *sin =
+ (struct sockaddr_in6 *)p->Address.lpSockaddr;
+ dst = (unsigned char *)sin->sin6_addr.s6_addr;
+ }
+
+ /* If non of the addresses are in correct format we will continue
+ * to next one */
+ if (src == 0 || dst == 0) {
+ continue;
+ }
+
+ /* Check if prefix network is the same network as we are testing
+ * start with whole bytes */
+
+ if (memcmp(src, dst, num_bytes) != 0) {
+ continue;
+ }
+
+ /* Check the remaning bits */
+ remaining_bits = p->PrefixLength % 8;
+
+ if (remaining_bits != 0) {
+ unsigned char mask = 0xff << (8 - remaining_bits);
+ int i = num_bytes;
+
+ if ((src[i] & mask) != (dst[i] & mask)) {
+ continue;
+ }
+ }
+
+ return p->PrefixLength;
+ }
+ }
+ return 0;
+}
+
+
+
GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
{
- error_set(errp, QERR_UNSUPPORTED);
- return NULL;
+ GuestNetworkInterfaceList *head = NULL, *curr_item = NULL;
+ DWORD ret_val;
+
+ PIP_ADAPTER_ADDRESSES adpt_addrs;
+ PIP_ADAPTER_ADDRESSES curr_adpt_addrs;
+ PIP_ADAPTER_UNICAST_ADDRESS adpt_uc_addr;
+
+ /* GetAdaptersAddresses requires ULONG */
+ ULONG bufLen = sizeof(IP_ADAPTER_ADDRESSES);
+
+ /* Startup WinSock32 */
+ WORD ws_version_requested = MAKEWORD(2, 2);
+ WSADATA ws_data;
+ WSAStartup(ws_version_requested, &ws_data);
+
+ /* Allocate adapter address list with one entry, used to
+ * fetch the read length needed by GetAdapterAddresses */
+ adpt_addrs = g_malloc(sizeof(IP_ADAPTER_ADDRESSES));
+
+ /* Get adapter adresses and if it doesn't fit in adpt_addrs
+ * we will realloc */
+ if (GetAdaptersAddresses(AF_UNSPEC,
+ GAA_FLAG_INCLUDE_PREFIX,
+ NULL,
+ adpt_addrs, &bufLen) ==
+ ERROR_BUFFER_OVERFLOW) {
+
+ g_free(adpt_addrs);
+ adpt_addrs = g_malloc(bufLen);
+ }
+
+ /* Get adapter address list */
+ ret_val = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
+ NULL,
+ adpt_addrs, &bufLen);
+ if (ret_val == NO_ERROR) {
+
+ /* Iterate all adapter addresses */
+ curr_adpt_addrs = adpt_addrs;
+
+ while (curr_adpt_addrs) {
+ /* Check if this adapter is up and running */
+
+ if (curr_adpt_addrs->OperStatus == IfOperStatusUp &&
+ (curr_adpt_addrs->IfType == IF_TYPE_ETHERNET_CSMACD ||
+ curr_adpt_addrs->IfType == IF_TYPE_IEEE80211 ||
+ curr_adpt_addrs->IfType == IF_TYPE_SOFTWARE_LOOPBACK)) {
+
+ int len = 0;
+ char *temp_description = 0;
+
+ GuestNetworkInterfaceList *interface_list =
+ g_malloc0(sizeof(*interface_list));
+ interface_list->value =
+ g_malloc0(sizeof(*interface_list->value));
+
+ len = wcslen(curr_adpt_addrs->Description) + 1;
+ temp_description = g_malloc(len);
+
+ wcstombs(temp_description,
+ curr_adpt_addrs->Description,
+ len);
+
+ interface_list->value->name = g_strdup(temp_description);
+
+ g_free(temp_description);
+
+ if (curr_item == NULL) {
+ head = curr_item = interface_list;
+ } else {
+ curr_item->next = interface_list;
+ curr_item = interface_list;
+ }
+
+ /* Format MAC address */
+ interface_list->value->hardware_address =
+ g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
+ (unsigned int) curr_adpt_addrs->PhysicalAddress[0],
+ (unsigned int) curr_adpt_addrs->PhysicalAddress[1],
+ (unsigned int) curr_adpt_addrs->PhysicalAddress[2],
+ (unsigned int) curr_adpt_addrs->PhysicalAddress[3],
+ (unsigned int) curr_adpt_addrs->PhysicalAddress[4],
+ (unsigned int) curr_adpt_addrs->PhysicalAddress[5]);
+
+
+ interface_list->value->has_hardware_address = true;
+
+ /* Iterate all unicast addresses of this adapter */
+ GuestIpAddressList **address_list =
+ &interface_list->value->ip_addresses;
+
+ GuestIpAddressList *address_item = NULL;
+
+ adpt_uc_addr = curr_adpt_addrs->FirstUnicastAddress;
+
+ while (adpt_uc_addr) {
+ SOCKET_ADDRESS *saddr = &adpt_uc_addr->Address;
+ int socklen;
+ char buffer[MAX_IPv6_LEN];
+ DWORD len = MAX_IPv6_LEN;
+
+ /* Allocate an address item */
+ address_item = g_malloc0(sizeof(*address_item));
+
+ address_item->value =
+ g_malloc0(sizeof(*address_item->value));
+
+ /* Is this IPv4 or IPv6 */
+ if (saddr->lpSockaddr->sa_family == AF_INET) {
+ address_item->value->ip_address_type =
+ GUEST_IP_ADDRESS_TYPE_IPV4;
+ socklen = sizeof(struct sockaddr_in);
+ } else if (saddr->lpSockaddr->sa_family == AF_INET6) {
+ address_item->value->ip_address_type =
+ GUEST_IP_ADDRESS_TYPE_IPV6;
+ socklen = sizeof(struct sockaddr_in6);
+ } else {
+ socklen = 0;
+ }
+
+ /* Temporary buffer to hold human readable address */
+ WSAAddressToString(saddr->lpSockaddr,
+ socklen, NULL, buffer, &len);
+ buffer[len] = 0;
+
+ address_item->value->ip_address = g_strdup(buffer);
+
+ /* Get the prefix for this address */
+ address_item->value->prefix =
+ get_prefix_length(curr_adpt_addrs->FirstPrefix,
+ saddr, socklen);
+
+ /* Add this address to the end of the address list */
+ while (*address_list && (*address_list)->next) {
+ address_list = &(*address_list)->next;
+ }
+
+ if (!*address_list) {
+ *address_list = address_item;
+ } else {
+ (*address_list)->next = address_item;
+ }
+
+ /* Jump to next address */
+ adpt_uc_addr = adpt_uc_addr->Next;
+ }
+ interface_list->value->has_ip_addresses = true;
+
+ }
+
+ /* Jump to next adapter */
+ curr_adpt_addrs = curr_adpt_addrs->Next;
+ }
+ }
+
+ /* Free the adapter list */
+ if (adpt_addrs != NULL) {
+ g_free(adpt_addrs);
+ }
+
+ /* Cleanup WinSock32 */
+ WSACleanup();
+
+ if (!head) {
+ error_set(errp,
+ ERROR_CLASS_GENERIC_ERROR,
+ "Could not get network interface information");
+ }
+
+ return head;
}
int64_t qmp_guest_get_time(Error **errp)
@@ -452,8 +692,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
const char *list_unsupported[] = {
"guest-file-open", "guest-file-close", "guest-file-read",
"guest-file-write", "guest-file-seek", "guest-file-flush",
- "guest-suspend-hybrid", "guest-network-get-interfaces",
- "guest-get-vcpus", "guest-set-vcpus",
+ "guest-suspend-hybrid", "guest-get-vcpus", "guest-set-vcpus",
"guest-fsfreeze-freeze-list", "guest-get-fsinfo",
"guest-fstrim", NULL};
char **p = (char **)list_unsupported;
--
1.9.3 (Apple Git-50)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: added windows support for 'guest-network-get-interfaces'
2014-09-29 18:22 [Qemu-devel] [PATCH v2] qemu-ga: added windows support for 'guest-network-get-interfaces' Kenth Andersson
@ 2014-10-01 17:45 ` Michael Roth
2014-10-08 7:13 ` Kenth Andersson
0 siblings, 1 reply; 3+ messages in thread
From: Michael Roth @ 2014-10-01 17:45 UTC (permalink / raw)
To: Kenth Andersson, Markus Armbruster; +Cc: QEMU
Quoting Kenth Andersson (2014-09-29 13:22:54)
> Implementation of guest-network-get-interfaces for windows
>
>
> Signed-off-by: Kenth Andersson <kenth@eastmark.net>
Thanks! I've been testing the functionality and it seems work nicely. Some
review comments below though:
> ---
> configure | 2 +-
> qga/commands-win32.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 244 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index 681abfc..7c6c60c 100755
> --- a/configure
> +++ b/configure
> @@ -717,7 +717,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=""
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3bcbeae..fb6fdba 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -14,6 +14,9 @@
> #include <glib.h>
> #include <wtypes.h>
> #include <powrprof.h>
> +#include <winsock2.h>
> +#include <iphlpapi.h>
> +#include <ws2tcpip.h>
> #include "qga/guest-agent-core.h"
> #include "qga/vss-win32.h"
> #include "qga-qmp-commands.h"
> @@ -29,6 +32,9 @@
> (365 * (1970 - 1601) + \
> (1970 - 1601) / 4 - 3))
>
> +/* Defines the max length of an IPv6 address in human readable format + pad */
> +#define MAX_IPv6_LEN 50
> +
> static void acquire_privilege(const char *name, Error **errp)
> {
> HANDLE token = NULL;
> @@ -359,10 +365,244 @@ void qmp_guest_suspend_hybrid(Error **errp)
> error_set(errp, QERR_UNSUPPORTED);
> }
>
> +static int get_prefix_length(PIP_ADAPTER_PREFIX prefix,
> + SOCKET_ADDRESS *sockaddr, int socklen)
> +{
> + PIP_ADAPTER_PREFIX p;
> + struct sockaddr *addr = sockaddr->lpSockaddr;
> +
> + for (p = prefix; p; p = p->Next) {
> + /* Compare the ip-adderss address family with the prefix
> + * address family */
> + if (addr->sa_family == p->Address.lpSockaddr->sa_family) {
> + int num_bytes = p->PrefixLength / 8;
> + unsigned char *src = 0;
> + unsigned char *dst = 0;
> + int remaining_bits;
> +
> + if (addr->sa_family == AF_INET) {
> + struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> + src = (unsigned char *)&(sin->sin_addr.s_addr);
> + } else if (addr->sa_family == AF_INET6) {
> + struct sockaddr_in6 *sin = (struct sockaddr_in6 *)addr;
> + src = (unsigned char *)sin->sin6_addr.s6_addr;
> + }
> +
> + if (p->Address.lpSockaddr->sa_family == AF_INET) {
> + struct sockaddr_in *sin =
> + (struct sockaddr_in *)p->Address.lpSockaddr;
> + dst = (unsigned char *)&(sin->sin_addr.s_addr);
> + } else if (p->Address.lpSockaddr->sa_family == AF_INET6) {
> + struct sockaddr_in6 *sin =
> + (struct sockaddr_in6 *)p->Address.lpSockaddr;
> + dst = (unsigned char *)sin->sin6_addr.s6_addr;
> + }
> +
> + /* If non of the addresses are in correct format we will continue
> + * to next one */
> + if (src == 0 || dst == 0) {
> + continue;
> + }
> +
> + /* Check if prefix network is the same network as we are testing
> + * start with whole bytes */
> +
> + if (memcmp(src, dst, num_bytes) != 0) {
> + continue;
> + }
> +
> + /* Check the remaning bits */
> + remaining_bits = p->PrefixLength % 8;
> +
> + if (remaining_bits != 0) {
> + unsigned char mask = 0xff << (8 - remaining_bits);
> + int i = num_bytes;
> +
> + if ((src[i] & mask) != (dst[i] & mask)) {
> + continue;
> + }
> + }
> +
> + return p->PrefixLength;
> + }
> + }
> + return 0;
> +}
> +
> +
> +
> GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
> {
> - error_set(errp, QERR_UNSUPPORTED);
> - return NULL;
> + GuestNetworkInterfaceList *head = NULL, *curr_item = NULL;
> + DWORD ret_val;
> +
> + PIP_ADAPTER_ADDRESSES adpt_addrs;
> + PIP_ADAPTER_ADDRESSES curr_adpt_addrs;
> + PIP_ADAPTER_UNICAST_ADDRESS adpt_uc_addr;
> +
> + /* GetAdaptersAddresses requires ULONG */
> + ULONG bufLen = sizeof(IP_ADAPTER_ADDRESSES);
buf_len
> +
> + /* Startup WinSock32 */
> + WORD ws_version_requested = MAKEWORD(2, 2);
> + WSADATA ws_data;
> + WSAStartup(ws_version_requested, &ws_data);
Since this can fail, we should return an error and bail (after calling WSACleanup).
Something like:
error_setg(errp, "failed to initialize Windows Socket API v2.2");
> +
> + /* Allocate adapter address list with one entry, used to
> + * fetch the read length needed by GetAdapterAddresses */
> + adpt_addrs = g_malloc(sizeof(IP_ADAPTER_ADDRESSES));
> +
> + /* Get adapter adresses and if it doesn't fit in adpt_addrs
addresses
> + * we will realloc */
> + if (GetAdaptersAddresses(AF_UNSPEC,
> + GAA_FLAG_INCLUDE_PREFIX,
> + NULL,
> + adpt_addrs, &bufLen) ==
> + ERROR_BUFFER_OVERFLOW) {
> +
> + g_free(adpt_addrs);
> + adpt_addrs = g_malloc(bufLen);
> + }
> +
> + /* Get adapter address list */
> + ret_val = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
> + NULL,
> + adpt_addrs, &bufLen);
Doesn't this mean we end up calling GetAdaptersAddresses unconditionally 2
times? Why not only make the second attempt if you get the overflow and
need to resize adpt_addrs?
Alternatively, maybe you can call GetAdaptersAddresses once with a NULL buffer
just to probe the size. This would still be 2 calls, but you can avoid
needing to guess at an initial size for adpt_addrs.
> + if (ret_val == NO_ERROR) {
I would just jump to error-handling if ret_val != NO_ERROR so you don't
need to indent the main block of code.
> +
> + /* Iterate all adapter addresses */
> + curr_adpt_addrs = adpt_addrs;
> +
> + while (curr_adpt_addrs) {
> + /* Check if this adapter is up and running */
> +
> + if (curr_adpt_addrs->OperStatus == IfOperStatusUp &&
> + (curr_adpt_addrs->IfType == IF_TYPE_ETHERNET_CSMACD ||
> + curr_adpt_addrs->IfType == IF_TYPE_IEEE80211 ||
> + curr_adpt_addrs->IfType == IF_TYPE_SOFTWARE_LOOPBACK)) {
Probably unlikely, but shouldn't IF_TYPE_PPP be included? And do we really
need to filter these at all? In the POSIX implementation we include all of
these, as well as IF_TYPE_TUNNEL (via tap/tun interfaces).
> +
> + int len = 0;
> + char *temp_description = 0;
> +
> + GuestNetworkInterfaceList *interface_list =
> + g_malloc0(sizeof(*interface_list));
> + interface_list->value =
> + g_malloc0(sizeof(*interface_list->value));
Small nit, but it gets a little to keep track of the pointers here. Maybe we
should just use sizeof(GuestNetworkInterfaceList) and
sizeof(GuestNetworkInterface) explicitly?
> +
> + len = wcslen(curr_adpt_addrs->Description) + 1;
> + temp_description = g_malloc(len);
> +
> + wcstombs(temp_description,
> + curr_adpt_addrs->Description,
> + len);
> +
> + interface_list->value->name = g_strdup(temp_description);
> +
> + g_free(temp_description);
> +
> + if (curr_item == NULL) {
> + head = curr_item = interface_list;
> + } else {
> + curr_item->next = interface_list;
> + curr_item = interface_list;
> + }
> +
> + /* Format MAC address */
> + interface_list->value->hardware_address =
> + g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
> + (unsigned int) curr_adpt_addrs->PhysicalAddress[0],
> + (unsigned int) curr_adpt_addrs->PhysicalAddress[1],
> + (unsigned int) curr_adpt_addrs->PhysicalAddress[2],
> + (unsigned int) curr_adpt_addrs->PhysicalAddress[3],
> + (unsigned int) curr_adpt_addrs->PhysicalAddress[4],
> + (unsigned int) curr_adpt_addrs->PhysicalAddress[5]);
> +
> +
> + interface_list->value->has_hardware_address = true;
> +
> + /* Iterate all unicast addresses of this adapter */
> + GuestIpAddressList **address_list =
> + &interface_list->value->ip_addresses;
> +
> + GuestIpAddressList *address_item = NULL;
> +
> + adpt_uc_addr = curr_adpt_addrs->FirstUnicastAddress;
> +
> + while (adpt_uc_addr) {
> + SOCKET_ADDRESS *saddr = &adpt_uc_addr->Address;
> + int socklen;
> + char buffer[MAX_IPv6_LEN];
> + DWORD len = MAX_IPv6_LEN;
> +
> + /* Allocate an address item */
> + address_item = g_malloc0(sizeof(*address_item));
> +
> + address_item->value =
> + g_malloc0(sizeof(*address_item->value));
> +
> + /* Is this IPv4 or IPv6 */
> + if (saddr->lpSockaddr->sa_family == AF_INET) {
> + address_item->value->ip_address_type =
> + GUEST_IP_ADDRESS_TYPE_IPV4;
> + socklen = sizeof(struct sockaddr_in);
> + } else if (saddr->lpSockaddr->sa_family == AF_INET6) {
> + address_item->value->ip_address_type =
> + GUEST_IP_ADDRESS_TYPE_IPV6;
> + socklen = sizeof(struct sockaddr_in6);
> + } else {
> + socklen = 0;
What's supposed to happen in this situation? Maybe we should just set
skip the address field in this case? And if we do, we should be carefully
about setting has_ip_addresses unconditionally below.
> + }
> +
> + /* Temporary buffer to hold human readable address */
> + WSAAddressToString(saddr->lpSockaddr,
> + socklen, NULL, buffer, &len);
> + buffer[len] = 0;
> +
> + address_item->value->ip_address = g_strdup(buffer);
> +
> + /* Get the prefix for this address */
> + address_item->value->prefix =
> + get_prefix_length(curr_adpt_addrs->FirstPrefix,
> + saddr, socklen);
> +
> + /* Add this address to the end of the address list */
> + while (*address_list && (*address_list)->next) {
> + address_list = &(*address_list)->next;
> + }
> +
> + if (!*address_list) {
> + *address_list = address_item;
> + } else {
> + (*address_list)->next = address_item;
> + }
Maybe something like this would be cleaner:
/* Iterate all unicast addresses of this adapter */
GuestIpAddressList *current_address_item = NULL;
adpt_uc_addr = curr_adpt_addrs->FirstUnicastAddress;
while (adpt_uc_addr) {
...
if (!current_address_item) {
interface_list->value->ip_addresses = address_item;
current_address_item = address_item;
} else {
current_address_item->next = address_item;
current_address_item = address_item;
}
}
In fact, I just noticed that's already how you're handling
GuestNetworkInterfaceList above.
> +
> + /* Jump to next address */
> + adpt_uc_addr = adpt_uc_addr->Next;
> + }
> + interface_list->value->has_ip_addresses = true;
> +
> + }
> +
> + /* Jump to next adapter */
> + curr_adpt_addrs = curr_adpt_addrs->Next;
> + }
> + }
> +
> + /* Free the adapter list */
> + if (adpt_addrs != NULL) {
> + g_free(adpt_addrs);
> + }
> +
> + /* Cleanup WinSock32 */
> + WSACleanup();
> +
> + if (!head) {
> + error_set(errp,
> + ERROR_CLASS_GENERIC_ERROR,
> + "Could not get network interface information");
you can use error_setg() for generic errors
> + }
> +
> + return head;
> }
>
> int64_t qmp_guest_get_time(Error **errp)
> @@ -452,8 +692,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
> const char *list_unsupported[] = {
> "guest-file-open", "guest-file-close", "guest-file-read",
> "guest-file-write", "guest-file-seek", "guest-file-flush",
> - "guest-suspend-hybrid", "guest-network-get-interfaces",
> - "guest-get-vcpus", "guest-set-vcpus",
> + "guest-suspend-hybrid", "guest-get-vcpus", "guest-set-vcpus",
> "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
> "guest-fstrim", NULL};
> char **p = (char **)list_unsupported;
> --
> 1.9.3 (Apple Git-50)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: added windows support for 'guest-network-get-interfaces'
2014-10-01 17:45 ` Michael Roth
@ 2014-10-08 7:13 ` Kenth Andersson
0 siblings, 0 replies; 3+ messages in thread
From: Kenth Andersson @ 2014-10-08 7:13 UTC (permalink / raw)
To: Michael Roth; +Cc: Markus Armbruster, QEMU
I will take care of the below items as soon as I get some free time.
/Kenth
On 01 Oct 2014, at 19:45, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Quoting Kenth Andersson (2014-09-29 13:22:54)
>> Implementation of guest-network-get-interfaces for windows
>>
>>
>> Signed-off-by: Kenth Andersson <kenth@eastmark.net>
>
> Thanks! I've been testing the functionality and it seems work nicely. Some
> review comments below though:
>
>> ---
>> configure | 2 +-
>> qga/commands-win32.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 244 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 681abfc..7c6c60c 100755
>> --- a/configure
>> +++ b/configure
>> @@ -717,7 +717,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=""
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 3bcbeae..fb6fdba 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -14,6 +14,9 @@
>> #include <glib.h>
>> #include <wtypes.h>
>> #include <powrprof.h>
>> +#include <winsock2.h>
>> +#include <iphlpapi.h>
>> +#include <ws2tcpip.h>
>> #include "qga/guest-agent-core.h"
>> #include "qga/vss-win32.h"
>> #include "qga-qmp-commands.h"
>> @@ -29,6 +32,9 @@
>> (365 * (1970 - 1601) + \
>> (1970 - 1601) / 4 - 3))
>>
>> +/* Defines the max length of an IPv6 address in human readable format + pad */
>> +#define MAX_IPv6_LEN 50
>> +
>> static void acquire_privilege(const char *name, Error **errp)
>> {
>> HANDLE token = NULL;
>> @@ -359,10 +365,244 @@ void qmp_guest_suspend_hybrid(Error **errp)
>> error_set(errp, QERR_UNSUPPORTED);
>> }
>>
>> +static int get_prefix_length(PIP_ADAPTER_PREFIX prefix,
>> + SOCKET_ADDRESS *sockaddr, int socklen)
>> +{
>> + PIP_ADAPTER_PREFIX p;
>> + struct sockaddr *addr = sockaddr->lpSockaddr;
>> +
>> + for (p = prefix; p; p = p->Next) {
>> + /* Compare the ip-adderss address family with the prefix
>> + * address family */
>> + if (addr->sa_family == p->Address.lpSockaddr->sa_family) {
>> + int num_bytes = p->PrefixLength / 8;
>> + unsigned char *src = 0;
>> + unsigned char *dst = 0;
>> + int remaining_bits;
>> +
>> + if (addr->sa_family == AF_INET) {
>> + struct sockaddr_in *sin = (struct sockaddr_in *)addr;
>> + src = (unsigned char *)&(sin->sin_addr.s_addr);
>> + } else if (addr->sa_family == AF_INET6) {
>> + struct sockaddr_in6 *sin = (struct sockaddr_in6 *)addr;
>> + src = (unsigned char *)sin->sin6_addr.s6_addr;
>> + }
>> +
>> + if (p->Address.lpSockaddr->sa_family == AF_INET) {
>> + struct sockaddr_in *sin =
>> + (struct sockaddr_in *)p->Address.lpSockaddr;
>> + dst = (unsigned char *)&(sin->sin_addr.s_addr);
>> + } else if (p->Address.lpSockaddr->sa_family == AF_INET6) {
>> + struct sockaddr_in6 *sin =
>> + (struct sockaddr_in6 *)p->Address.lpSockaddr;
>> + dst = (unsigned char *)sin->sin6_addr.s6_addr;
>> + }
>> +
>> + /* If non of the addresses are in correct format we will continue
>> + * to next one */
>> + if (src == 0 || dst == 0) {
>> + continue;
>> + }
>> +
>> + /* Check if prefix network is the same network as we are testing
>> + * start with whole bytes */
>> +
>> + if (memcmp(src, dst, num_bytes) != 0) {
>> + continue;
>> + }
>> +
>> + /* Check the remaning bits */
>> + remaining_bits = p->PrefixLength % 8;
>> +
>> + if (remaining_bits != 0) {
>> + unsigned char mask = 0xff << (8 - remaining_bits);
>> + int i = num_bytes;
>> +
>> + if ((src[i] & mask) != (dst[i] & mask)) {
>> + continue;
>> + }
>> + }
>> +
>> + return p->PrefixLength;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +
>> +
>> GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>> {
>> - error_set(errp, QERR_UNSUPPORTED);
>> - return NULL;
>> + GuestNetworkInterfaceList *head = NULL, *curr_item = NULL;
>> + DWORD ret_val;
>> +
>> + PIP_ADAPTER_ADDRESSES adpt_addrs;
>> + PIP_ADAPTER_ADDRESSES curr_adpt_addrs;
>> + PIP_ADAPTER_UNICAST_ADDRESS adpt_uc_addr;
>> +
>> + /* GetAdaptersAddresses requires ULONG */
>> + ULONG bufLen = sizeof(IP_ADAPTER_ADDRESSES);
>
> buf_len
>
>> +
>> + /* Startup WinSock32 */
>> + WORD ws_version_requested = MAKEWORD(2, 2);
>> + WSADATA ws_data;
>> + WSAStartup(ws_version_requested, &ws_data);
>
> Since this can fail, we should return an error and bail (after calling WSACleanup).
> Something like:
>
> error_setg(errp, "failed to initialize Windows Socket API v2.2");
>
>> +
>> + /* Allocate adapter address list with one entry, used to
>> + * fetch the read length needed by GetAdapterAddresses */
>> + adpt_addrs = g_malloc(sizeof(IP_ADAPTER_ADDRESSES));
>> +
>> + /* Get adapter adresses and if it doesn't fit in adpt_addrs
>
> addresses
>
>> + * we will realloc */
>> + if (GetAdaptersAddresses(AF_UNSPEC,
>> + GAA_FLAG_INCLUDE_PREFIX,
>> + NULL,
>> + adpt_addrs, &bufLen) ==
>> + ERROR_BUFFER_OVERFLOW) {
>> +
>> + g_free(adpt_addrs);
>> + adpt_addrs = g_malloc(bufLen);
>> + }
>> +
>> + /* Get adapter address list */
>> + ret_val = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
>> + NULL,
>> + adpt_addrs, &bufLen);
>
> Doesn't this mean we end up calling GetAdaptersAddresses unconditionally 2
> times? Why not only make the second attempt if you get the overflow and
> need to resize adpt_addrs?
>
> Alternatively, maybe you can call GetAdaptersAddresses once with a NULL buffer
> just to probe the size. This would still be 2 calls, but you can avoid
> needing to guess at an initial size for adpt_addrs.
>
>> + if (ret_val == NO_ERROR) {
>
> I would just jump to error-handling if ret_val != NO_ERROR so you don't
> need to indent the main block of code.
>
>> +
>> + /* Iterate all adapter addresses */
>> + curr_adpt_addrs = adpt_addrs;
>> +
>> + while (curr_adpt_addrs) {
>> + /* Check if this adapter is up and running */
>> +
>> + if (curr_adpt_addrs->OperStatus == IfOperStatusUp &&
>> + (curr_adpt_addrs->IfType == IF_TYPE_ETHERNET_CSMACD ||
>> + curr_adpt_addrs->IfType == IF_TYPE_IEEE80211 ||
>> + curr_adpt_addrs->IfType == IF_TYPE_SOFTWARE_LOOPBACK)) {
>
> Probably unlikely, but shouldn't IF_TYPE_PPP be included? And do we really
> need to filter these at all? In the POSIX implementation we include all of
> these, as well as IF_TYPE_TUNNEL (via tap/tun interfaces).
>
>> +
>> + int len = 0;
>> + char *temp_description = 0;
>> +
>> + GuestNetworkInterfaceList *interface_list =
>> + g_malloc0(sizeof(*interface_list));
>> + interface_list->value =
>> + g_malloc0(sizeof(*interface_list->value));
>
> Small nit, but it gets a little to keep track of the pointers here. Maybe we
> should just use sizeof(GuestNetworkInterfaceList) and
> sizeof(GuestNetworkInterface) explicitly?
>
>> +
>> + len = wcslen(curr_adpt_addrs->Description) + 1;
>> + temp_description = g_malloc(len);
>> +
>> + wcstombs(temp_description,
>> + curr_adpt_addrs->Description,
>> + len);
>> +
>> + interface_list->value->name = g_strdup(temp_description);
>> +
>> + g_free(temp_description);
>> +
>> + if (curr_item == NULL) {
>> + head = curr_item = interface_list;
>> + } else {
>> + curr_item->next = interface_list;
>> + curr_item = interface_list;
>> + }
>> +
>> + /* Format MAC address */
>> + interface_list->value->hardware_address =
>> + g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
>> + (unsigned int) curr_adpt_addrs->PhysicalAddress[0],
>> + (unsigned int) curr_adpt_addrs->PhysicalAddress[1],
>> + (unsigned int) curr_adpt_addrs->PhysicalAddress[2],
>> + (unsigned int) curr_adpt_addrs->PhysicalAddress[3],
>> + (unsigned int) curr_adpt_addrs->PhysicalAddress[4],
>> + (unsigned int) curr_adpt_addrs->PhysicalAddress[5]);
>> +
>> +
>> + interface_list->value->has_hardware_address = true;
>> +
>> + /* Iterate all unicast addresses of this adapter */
>> + GuestIpAddressList **address_list =
>> + &interface_list->value->ip_addresses;
>> +
>> + GuestIpAddressList *address_item = NULL;
>> +
>> + adpt_uc_addr = curr_adpt_addrs->FirstUnicastAddress;
>> +
>> + while (adpt_uc_addr) {
>> + SOCKET_ADDRESS *saddr = &adpt_uc_addr->Address;
>> + int socklen;
>> + char buffer[MAX_IPv6_LEN];
>> + DWORD len = MAX_IPv6_LEN;
>> +
>> + /* Allocate an address item */
>> + address_item = g_malloc0(sizeof(*address_item));
>> +
>> + address_item->value =
>> + g_malloc0(sizeof(*address_item->value));
>> +
>> + /* Is this IPv4 or IPv6 */
>> + if (saddr->lpSockaddr->sa_family == AF_INET) {
>> + address_item->value->ip_address_type =
>> + GUEST_IP_ADDRESS_TYPE_IPV4;
>> + socklen = sizeof(struct sockaddr_in);
>> + } else if (saddr->lpSockaddr->sa_family == AF_INET6) {
>> + address_item->value->ip_address_type =
>> + GUEST_IP_ADDRESS_TYPE_IPV6;
>> + socklen = sizeof(struct sockaddr_in6);
>> + } else {
>> + socklen = 0;
>
> What's supposed to happen in this situation? Maybe we should just set
> skip the address field in this case? And if we do, we should be carefully
> about setting has_ip_addresses unconditionally below.
>
>> + }
>> +
>> + /* Temporary buffer to hold human readable address */
>> + WSAAddressToString(saddr->lpSockaddr,
>> + socklen, NULL, buffer, &len);
>> + buffer[len] = 0;
>> +
>> + address_item->value->ip_address = g_strdup(buffer);
>> +
>> + /* Get the prefix for this address */
>> + address_item->value->prefix =
>> + get_prefix_length(curr_adpt_addrs->FirstPrefix,
>> + saddr, socklen);
>
>
>> +
>> + /* Add this address to the end of the address list */
>> + while (*address_list && (*address_list)->next) {
>> + address_list = &(*address_list)->next;
>> + }
>> +
>> + if (!*address_list) {
>> + *address_list = address_item;
>> + } else {
>> + (*address_list)->next = address_item;
>> + }
>
> Maybe something like this would be cleaner:
>
> /* Iterate all unicast addresses of this adapter */
> GuestIpAddressList *current_address_item = NULL;
>
> adpt_uc_addr = curr_adpt_addrs->FirstUnicastAddress;
>
> while (adpt_uc_addr) {
> ...
>
> if (!current_address_item) {
> interface_list->value->ip_addresses = address_item;
> current_address_item = address_item;
> } else {
> current_address_item->next = address_item;
> current_address_item = address_item;
> }
> }
>
> In fact, I just noticed that's already how you're handling
> GuestNetworkInterfaceList above.
>
>> +
>> + /* Jump to next address */
>> + adpt_uc_addr = adpt_uc_addr->Next;
>> + }
>> + interface_list->value->has_ip_addresses = true;
>> +
>> + }
>> +
>> + /* Jump to next adapter */
>> + curr_adpt_addrs = curr_adpt_addrs->Next;
>> + }
>> + }
>> +
>> + /* Free the adapter list */
>> + if (adpt_addrs != NULL) {
>> + g_free(adpt_addrs);
>> + }
>> +
>> + /* Cleanup WinSock32 */
>> + WSACleanup();
>> +
>> + if (!head) {
>> + error_set(errp,
>> + ERROR_CLASS_GENERIC_ERROR,
>> + "Could not get network interface information");
>
> you can use error_setg() for generic errors
>
>> + }
>> +
>> + return head;
>> }
>>
>> int64_t qmp_guest_get_time(Error **errp)
>> @@ -452,8 +692,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
>> const char *list_unsupported[] = {
>> "guest-file-open", "guest-file-close", "guest-file-read",
>> "guest-file-write", "guest-file-seek", "guest-file-flush",
>> - "guest-suspend-hybrid", "guest-network-get-interfaces",
>> - "guest-get-vcpus", "guest-set-vcpus",
>> + "guest-suspend-hybrid", "guest-get-vcpus", "guest-set-vcpus",
>> "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
>> "guest-fstrim", NULL};
>> char **p = (char **)list_unsupported;
>> --
>> 1.9.3 (Apple Git-50)
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-08 7:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-29 18:22 [Qemu-devel] [PATCH v2] qemu-ga: added windows support for 'guest-network-get-interfaces' Kenth Andersson
2014-10-01 17:45 ` Michael Roth
2014-10-08 7:13 ` Kenth Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).