Quoting Kenth Andersson (2014-09-29 13:22:54)
> Implementation of guest-network-get-interfaces for windows
>
>
> Signed-off-by: Kenth Andersson <ke...@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)
