From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46346) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZNyv-0001sJ-Pj for qemu-devel@nongnu.org; Wed, 01 Oct 2014 13:46:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZNyo-00018p-1Y for qemu-devel@nongnu.org; Wed, 01 Oct 2014 13:46:29 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:51495) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZNyn-00018j-Sj for qemu-devel@nongnu.org; Wed, 01 Oct 2014 13:46:21 -0400 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 1 Oct 2014 13:46:21 -0400 Received: from b01cxnp23032.gho.pok.ibm.com (b01cxnp23032.gho.pok.ibm.com [9.57.198.27]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 012626E8028 for ; Wed, 1 Oct 2014 13:35:04 -0400 (EDT) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by b01cxnp23032.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id s91HkBxX65142852 for ; Wed, 1 Oct 2014 17:46:19 GMT Received: from d01av05.pok.ibm.com (localhost [127.0.0.1]) by d01av05.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s91Hjkxk027855 for ; Wed, 1 Oct 2014 13:45:46 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <098E614A-9C31-4FCB-9F79-5FFC0AA97867@eastmark.net> References: <098E614A-9C31-4FCB-9F79-5FFC0AA97867@eastmark.net> Message-ID: <20141001174529.19243.1359@loki> Date: Wed, 01 Oct 2014 12:45:29 -0500 Subject: Re: [Qemu-devel] [PATCH v2] qemu-ga: added windows support for 'guest-network-get-interfaces' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 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=3D"\${prefix}" > local_statedir=3D > confsuffix=3D"" > - libs_qga=3D"-lws2_32 -lwinmm -lpowrprof $libs_qga" > + libs_qga=3D"-lws2_32 -lwinmm -lpowrprof -liphlpapi $libs_qga" > fi > = > werror=3D"" > 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 > #include > #include > +#include > +#include > +#include > #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 =3D 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 =3D sockaddr->lpSockaddr; > + > + for (p =3D prefix; p; p =3D p->Next) { > + /* Compare the ip-adderss address family with the prefix > + * address family */ > + if (addr->sa_family =3D=3D p->Address.lpSockaddr->sa_family) { > + int num_bytes =3D p->PrefixLength / 8; > + unsigned char *src =3D 0; > + unsigned char *dst =3D 0; > + int remaining_bits; > + > + if (addr->sa_family =3D=3D AF_INET) { > + struct sockaddr_in *sin =3D (struct sockaddr_in *)addr; > + src =3D (unsigned char *)&(sin->sin_addr.s_addr); > + } else if (addr->sa_family =3D=3D AF_INET6) { > + struct sockaddr_in6 *sin =3D (struct sockaddr_in6 *)addr; > + src =3D (unsigned char *)sin->sin6_addr.s6_addr; > + } > + > + if (p->Address.lpSockaddr->sa_family =3D=3D AF_INET) { > + struct sockaddr_in *sin =3D > + (struct sockaddr_in *)p->Address.lpSockaddr; > + dst =3D (unsigned char *)&(sin->sin_addr.s_addr); > + } else if (p->Address.lpSockaddr->sa_family =3D=3D AF_INET6)= { > + struct sockaddr_in6 *sin =3D > + (struct sockaddr_in6 *)p->Address.lpSockaddr; > + dst =3D (unsigned char *)sin->sin6_addr.s6_addr; > + } > + > + /* If non of the addresses are in correct format we will con= tinue > + * to next one */ > + if (src =3D=3D 0 || dst =3D=3D 0) { > + continue; > + } > + > + /* Check if prefix network is the same network as we are tes= ting > + * start with whole bytes */ > + > + if (memcmp(src, dst, num_bytes) !=3D 0) { > + continue; > + } > + > + /* Check the remaning bits */ > + remaining_bits =3D p->PrefixLength % 8; > + > + if (remaining_bits !=3D 0) { > + unsigned char mask =3D 0xff << (8 - remaining_bits); > + int i =3D num_bytes; > + > + if ((src[i] & mask) !=3D (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 =3D NULL, *curr_item =3D 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 =3D sizeof(IP_ADAPTER_ADDRESSES); buf_len > + > + /* Startup WinSock32 */ > + WORD ws_version_requested =3D 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 WSAC= leanup). 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 =3D 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) =3D=3D > + ERROR_BUFFER_OVERFLOW) { > + > + g_free(adpt_addrs); > + adpt_addrs =3D g_malloc(bufLen); > + } > + > + /* Get adapter address list */ > + ret_val =3D 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 buf= fer 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 =3D=3D NO_ERROR) { I would just jump to error-handling if ret_val !=3D NO_ERROR so you don't need to indent the main block of code. > + > + /* Iterate all adapter addresses */ > + curr_adpt_addrs =3D adpt_addrs; > + > + while (curr_adpt_addrs) { > + /* Check if this adapter is up and running */ > + > + if (curr_adpt_addrs->OperStatus =3D=3D IfOperStatusUp && > + (curr_adpt_addrs->IfType =3D=3D IF_TYPE_ETHERNET_CSMACD = || > + curr_adpt_addrs->IfType =3D=3D IF_TYPE_IEEE80211 || > + curr_adpt_addrs->IfType =3D=3D IF_TYPE_SOFTWARE_LOOPBAC= K)) { 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 =3D 0; > + char *temp_description =3D 0; > + > + GuestNetworkInterfaceList *interface_list =3D > + g_malloc0(sizeof(*interface_list)); > + interface_list->value =3D > + 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 =3D wcslen(curr_adpt_addrs->Description) + 1; > + temp_description =3D g_malloc(len); > + > + wcstombs(temp_description, > + curr_adpt_addrs->Description, > + len); > + > + interface_list->value->name =3D g_strdup(temp_descriptio= n); > + > + g_free(temp_description); > + > + if (curr_item =3D=3D NULL) { > + head =3D curr_item =3D interface_list; > + } else { > + curr_item->next =3D interface_list; > + curr_item =3D interface_list; > + } > + > + /* Format MAC address */ > + interface_list->value->hardware_address =3D > + 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 =3D true; > + > + /* Iterate all unicast addresses of this adapter */ > + GuestIpAddressList **address_list =3D > + &interface_list->value->ip_addre= sses; > + > + GuestIpAddressList *address_item =3D NULL; > + > + adpt_uc_addr =3D curr_adpt_addrs->FirstUnicastAddress; > + > + while (adpt_uc_addr) { > + SOCKET_ADDRESS *saddr =3D &adpt_uc_addr->Address; > + int socklen; > + char buffer[MAX_IPv6_LEN]; > + DWORD len =3D MAX_IPv6_LEN; > + > + /* Allocate an address item */ > + address_item =3D g_malloc0(sizeof(*address_item)); > + > + address_item->value =3D > + g_malloc0(sizeof(*address_item->valu= e)); > + > + /* Is this IPv4 or IPv6 */ > + if (saddr->lpSockaddr->sa_family =3D=3D AF_INET) { > + address_item->value->ip_address_type =3D > + GUEST_IP_ADDRESS_TYPE_IP= V4; > + socklen =3D sizeof(struct sockaddr_in); > + } else if (saddr->lpSockaddr->sa_family =3D=3D AF_IN= ET6) { > + address_item->value->ip_address_type =3D > + GUEST_IP_ADDRESS_TYPE_IP= V6; > + socklen =3D sizeof(struct sockaddr_in6); > + } else { > + socklen =3D 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] =3D 0; > + > + address_item->value->ip_address =3D g_strdup(buffer); > + > + /* Get the prefix for this address */ > + address_item->value->prefix =3D > + get_prefix_length(curr_adpt_addrs->FirstPr= efix, > + saddr, socklen); > + > + /* Add this address to the end of the address list */ > + while (*address_list && (*address_list)->next) { > + address_list =3D &(*address_list)->next; > + } > + > + if (!*address_list) { > + *address_list =3D address_item; > + } else { > + (*address_list)->next =3D address_item; > + } Maybe something like this would be cleaner: /* Iterate all unicast addresses of this adapter */ GuestIpAddressList *current_address_item =3D NULL; adpt_uc_addr =3D curr_adpt_addrs->FirstUnicastAddress; while (adpt_uc_addr) { ... if (!current_address_item) { interface_list->value->ip_addresses =3D address_= item; current_address_item =3D address_item; } else { current_address_item->next =3D address_item; current_address_item =3D address_item; } } In fact, I just noticed that's already how you're handling GuestNetworkInterfaceList above. > + > + /* Jump to next address */ > + adpt_uc_addr =3D adpt_uc_addr->Next; > + } > + interface_list->value->has_ip_addresses =3D true; > + > + } > + > + /* Jump to next adapter */ > + curr_adpt_addrs =3D curr_adpt_addrs->Next; > + } > + } > + > + /* Free the adapter list */ > + if (adpt_addrs !=3D 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[] =3D { > "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 =3D (char **)list_unsupported; > -- = > 1.9.3 (Apple Git-50)