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 <mdr...@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 <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)
>
>
