Re: [Qemu-devel] [PATCH] qemu-ga: added windows support for 'guest-network-get-interfaces'

[Markus Armbruster]

Kenth Andersson <address@hidden> writes:

> This is my first patch so I hope I got everything right.

Here, this sentence is part of the commit message.  It should go...

> Signed-off-by: Kenth Andersson <address@hidden>
> ---

... here, below the '---' divider.

As a happy Windows ignoramus, I can't do a real review, but I'll try to
give a few hints.

>  configure            |   2 +-
>  qga/commands-win32.c | 267 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 266 insertions(+), 3 deletions(-)
>
> diff --git a/configure b/configure
> index 862f6d2..1d2aeae 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..e084573 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"
> @@ -359,10 +362,270 @@ void qmp_guest_suspend_hybrid(Error **errp)
>      error_set(errp, QERR_UNSUPPORTED);
>  }
>  
> +static int get_prefix(PIP_ADAPTER_PREFIX prefix,
> +                      LPSOCKADDR sockaddr, int socklen)
> +{
> +    PIP_ADAPTER_PREFIX p = prefix;
> +
> +    for (; p != NULL; p = p->Next)
> +    {

Coding style: the brace should be on the previous line.  Please feed
your patch to scripts/checkpatch, and address those of its complaints
that make sense.  I won't flag further style issues checkpatch also
flags.

Please initialize the loop control variable in the for, like this:

    PIP_ADAPTER_PREFIX p;

    for (p = prefix; p; p = p->Next)

> +        if (sockaddr->sa_family == p->Address.lpSockaddr->sa_family)
> +        {
> +            int num_bytes = p->PrefixLength / 8;
> +            int i = 0;
> +            char* src = 0;
> +            char* dst = 0;
> +            int remaining_bits = 0;

Initialization of remaining_bits is dead code.  I don't like that
because

1. When you later add a use between the dead and the real
initialization, but forgets to also add a real initialization, the
compiler won't warn.  Instead the now dead initialization silently
becomes alive, and may not be what you want.

2. Dead initializations can get flagged by really picky compilers or
static analyzers, cluttering their diagnostics.

> +
> +            if (sockaddr->sa_family == AF_INET)
> +            {
> +                struct sockaddr_in* sin = (struct sockaddr_in*)sockaddr;
> +                src = (char*)sin->sin_addr.s_addr;
> +            }
> +            else if (sockaddr->sa_family == AF_INET6)
> +            {
> +                struct sockaddr_in6* sin = (struct sockaddr_in6*)sockaddr;
> +                src = (char*)sin->sin6_addr.s6_addr;
> +            }
> +            if (prefix->Address.lpSockaddr->sa_family == AF_INET)
> +            {
> +                struct sockaddr_in* sin =
> +                            (struct sockaddr_in*)prefix->Address.lpSockaddr;
> +                dst = (char*)sin->sin_addr.s_addr;
> +            }
> +            else if (prefix->Address.lpSockaddr->sa_family == AF_INET6)
> +            {
> +                struct sockaddr_in6* sin =
> +                            (struct sockaddr_in6*)prefix->Address.lpSockaddr;
> +                dst = (char*)sin->sin6_addr.s6_addr;
> +            }

src and dst are char *, but they aren't strings.  I feel it's better to
reserve char * for strings, and use either void * or unsigned char * for
pointers to blobs.  Whatever requires less type casting.

> +            if ((src == 0) || (dst == 0))

Redundant parenthesis.  Existing code in this file doesn't use them.
Please make your code blend in.  I'm not going to flag further instances
of the same issue.

> +            {
> +                continue;
> +            }
> +
> +            for (; i < num_bytes; ++i)
> +            {
> +                if (src[i] != dst[i])
> +                {
> +                    continue;
> +                }
> +            }

Consider memcmp().

> +
> +            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;
> +                }
> +            }

Can sockaddr->sa_family != prefix->Address.lpSockaddr->sa_family happen?

If yes, you're comparing apples with oranges then.

> +
> +            return p->PrefixLength;
> +        }
> +    }
> +    return 0;
> +}

Despite its name, this function doesn't seem to get a prefix, but a
prefix length.

> +
> +
> +
>  GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>  {
> -    error_set(errp, QERR_UNSUPPORTED);
> -    return NULL;
> +    GuestNetworkInterfaceList *head = NULL, *curr_item = NULL;
> +    DWORD ret_val = 0;

Another dead initialization.

> +
> +    /* Startup WinSock32 */
> +    WORD ws_version_requested = MAKEWORD(2, 2);
> +    WSADATA ws_data;
> +    WSAStartup(ws_version_requested, &ws_data);
> +
> +
> +    PIP_ADAPTER_ADDRESSES adapter_addresses = NULL;
> +    PIP_ADAPTER_ADDRESSES current_adapter_address = NULL;
> +    PIP_ADAPTER_UNICAST_ADDRESS adapter_unicast_address = NULL;

Declarations follow statements.  Please don't do that.

More dead initializations.

These are mightly long names for such modest local variables.  Matter of
taste.

> +
> +    unsigned long bufLen = sizeof(IP_ADAPTER_ADDRESSES);

Shouldn't this be size_t?

> +
> +    /* Allocate adapter address list */
> +    adapter_addresses = (IP_ADAPTER_ADDRESSES*)
> +                HeapAlloc(GetProcessHeap(), 0, sizeof(IP_ADAPTER_ADDRESSES));

Please don't cast from void * needlessly.  This isn't C++.  The cast
clutters the code and reduces your chance for a compiler warning when
you fatfinger something.

Sure you need HeapAlloc()?  Why wouldn't g_new(IP_ADAPTER_ADDRESSES) do?

> +    if (adapter_addresses == NULL) {
> +        goto cleanup;

Is this a failure?  If yes, you need to error_setg(errp, ...).

Same question for all the other goto cleanup.

> +    }
> +
> +    /* Get adapter adresses and if it doesn't fit in adapter_addresses
> +     * we will realloc */
> +    if (GetAdaptersAddresses(AF_UNSPEC,
> +                             GAA_FLAG_INCLUDE_PREFIX,
> +                             NULL,
> +                             adapter_addresses, &bufLen) ==
> +                                            ERROR_BUFFER_OVERFLOW) {
> +
> +        HeapFree(GetProcessHeap(), 0, adapter_addresses);
> +
> +        adapter_addresses = (IP_ADAPTER_ADDRESSES*)
> +                    HeapAlloc(GetProcessHeap(), 0, bufLen);
> +
> +        if (adapter_addresses == NULL) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    /* Get adapter address list */
> +    if ((ret_val = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
> +                                  NULL,
> +                                  adapter_addresses, &bufLen)) == NO_ERROR) {

Having the assignment in the condition isn't necessary.  Please

    ret_val = ...
    if (ret_val == NO_ERROR) {

> +
> +        /* Iterate all adapter addresses */
> +
> +        current_adapter_address = adapter_addresses;
> +
> +        while (current_adapter_address) {
> +            /* Check if this adapter is up and running */
> +
> +            if ((current_adapter_address->OperStatus == IfOperStatusUp) &&
> +                ((current_adapter_address->IfType == 
> IF_TYPE_ETHERNET_CSMACD) ||
> +                 (current_adapter_address->IfType == IF_TYPE_IEEE80211) ||
> +                 (current_adapter_address->IfType ==
> +                                            IF_TYPE_SOFTWARE_LOOPBACK))) {
> +
> +                int len = 0;
> +                char* temp_description = 0;
> +
> +                GuestNetworkInterfaceList* interface_list =
> +                                            
> g_malloc0(sizeof(*interface_list));

Recommend to indent the continued line a bit less, to keep line length
down.

> +                if (interface_list == NULL) {
> +                    goto cleanup;
> +                }

g_malloc0() returns null only when its argument is zero, so this error
handling is useless clutter.  Please drop.  Same elsewhere.

> +
> +                interface_list->value = 
> g_malloc0(sizeof(*interface_list->value));
> +                if (interface_list->value == NULL) {
> +                    g_free(interface_list);
> +                    goto cleanup;
> +                }
> +
> +                len = wcslen(current_adapter_address->Description) + 1;
> +                temp_description = g_malloc0(len);

Sure you need *temp_description zeroed?  Its use below...

> +                if (temp_description == NULL) {
> +                    g_free(interface_list->value);
> +                    g_free(interface_list);
> +                    goto cleanup;
> +                }
> +
> +                wcstombs(temp_description,
> +                         current_adapter_address->Description,
> +                         len);

... appears to fill it just fine.

> +                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",
> +                            (int) 
> current_adapter_address->PhysicalAddress[0],
> +                            (int) 
> current_adapter_address->PhysicalAddress[1],
> +                            (int) 
> current_adapter_address->PhysicalAddress[2],
> +                            (int) 
> current_adapter_address->PhysicalAddress[3],
> +                            (int) 
> current_adapter_address->PhysicalAddress[4],
> +                            (int) 
> current_adapter_address->PhysicalAddress[5]);

Assuming the casts are necessary: casting to unsigned int would be
cleaner, because %02x expects an unsigned int argument.

Hmm, you're merely following qga/commands-posix.c precedent here.

> +
> +
> +                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;
> +
> +                adapter_unicast_address =
> +                        current_adapter_address->FirstUnicastAddress;
> +
> +                while (adapter_unicast_address) {
> +                    SOCKET_ADDRESS* saddr = 
> &adapter_unicast_address->Address;
> +                    int socklen = 0;
> +                    char buffer[50];
> +                    DWORD len = 50;

Where does 50 come from?

> +
> +                    /* Allocate an address item */
> +                    address_item = g_malloc0(sizeof(*address_item));
> +                    if (address_item == NULL) {
> +                        goto cleanup;
> +                    }
> +                    address_item->value =
> +                                    g_malloc0(sizeof(*address_item->value));
> +
> +                    if (address_item->value == NULL) {
> +                        g_free(address_item);
> +                        goto cleanup;
> +                    }
> +
> +                    /* 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);
> +                    }
> +
> +                    /* 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(current_adapter_address->FirstPrefix,
> +                                         saddr->lpSockaddr, socklen);
> +
> +                    /* Add this address to the end of the address list */
> +                    while (*address_list && (*address_list)->next) {
> +                        address_list = &(*address_list)->next;
> +                    }

Suggest a for loop.

> +
> +                    if (!*address_list) {
> +                        *address_list = address_item;
> +                    } else {
> +                        (*address_list)->next = address_item;
> +                    }
> +
> +                    /* Jump to next address */
> +                    adapter_unicast_address = adapter_unicast_address->Next;
> +                }
> +                interface_list->value->has_ip_addresses = true;
> +
> +            }
> +
> +            /* Jump to next adapter */
> +            current_adapter_address = current_adapter_address->Next;
> +        }
> +    }
> +
> +cleanup:
> +
> +    /* Free the adapter list */
> +    if (adapter_addresses != NULL) {
> +        HeapFree(GetProcessHeap(), 0, adapter_addresses);
> +    }
> +
> +    /* Cleanup WinSock32 */
> +    WSACleanup();
> +
> +    return head;
>  }
>  
>  int64_t qmp_guest_get_time(Error **errp)

Looks like pretty solid work to me.  My numerous comments mostly show
your code needs a bit of adapting to better blend with the existing QEMU
code.