Thanks for the feedback. I'll make the necessary adjustments and create a new
patch.
Kirk
>>>
> On 01/04/15 18:39, Kirk Allan wrote:
>> By default, IP addresses and prefixes will be derived from information
>> obtained by various calls and structures. IPv4 prefixes can be found
>> by matching the address to those returned by GetApaptersInfo. IPv6
>> prefixes can not be matched this way due to the unpredictable order of
>> entries.
> GetAdaptersInfo.
I'll fix the typo.
>> In Windows Visa/2008 guests and newer, it is possible to use inet_ntop()
>> and OnLinkPrefixLength to get IPv4 and IPv6 addresses and prefixes.
>> Setting *extra-cflags in the build configuration to
>> *- D_WIN32_WINNT-0x600 -DWINVER=0x600* or greater enables this functionality
>> for those guests. Setting *ectra-cflags is not required and if not used,
>> the default approach will be taken.
>>
>> Signed-off-by: Kirk Allan <kal...@suse.com>
>> ---
>> qga/commands-win32.c | 319
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 317 insertions(+), 2 deletions(-)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 3ef0549..635d3ca 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -16,11 +16,17 @@
>> #include <powrprof.h>
>> #include <stdio.h>
>> #include <string.h>
>> +#include <winsock2.h>
>> +#include <ws2tcpip.h>
>> +#include <ws2ipdef.h>
>> +#include <iptypes.h>
>> +#include <iphlpapi.h>
>> #include "qga/guest-agent-core.h"
>> #include "qga/vss-win32.h"
>> #include "qga-qmp-commands.h"
>> #include "qapi/qmp/qerror.h"
>> #include "qemu/queue.h"
>> +#include "qemu/host-utils.h"
>>
>> #ifndef SHTDN_REASON_FLAG_PLANNED
>> #define SHTDN_REASON_FLAG_PLANNED 0x80000000
>> @@ -589,9 +595,318 @@ void qmp_guest_suspend_hybrid(Error **errp)
>> error_set(errp, QERR_UNSUPPORTED);
>> }
>>
>> +#define WORKING_BUFFER_SIZE 15000
>> +#define MAX_ALLOC_TRIES 2
>> +
>> +static DWORD guest_get_adapter_addresses(IP_ADAPTER_ADDRESSES
> **adptr_addrs)
>> +{
>> + ULONG out_buf_len = 0;
>> + int alloc_tries = 0;
>> + DWORD ret = ERROR_SUCCESS;
>> +
>> + /* Allocate a 15 KB buffer to start with. If not enough, out_buf_len
>> + * will have the amount needed after the call to GetAdaptersAddresses.
>> + */
> may be we should not allocate memory in the beginning and just make a
> call with just pointer,
> and get necessary size.
I'll get the size first and make one allocation.
>> + out_buf_len = WORKING_BUFFER_SIZE;
>> +
>> + do {
>> + *adptr_addrs = (IP_ADAPTER_ADDRESSES *) g_malloc(out_buf_len);
>> + if (*adptr_addrs == NULL) {
>> + return ERROR_NOT_ENOUGH_MEMORY;
> I think we should use error handling with win32 macro error_setg_win32(
> errno, GetLastError()..
I'll use error_setg_win32() when the api sets last error.
>> + }
>> +
>> + ret = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_INCLUDE_PREFIX,
>> + NULL, *adptr_addrs, &out_buf_len);
>> +
>> + if (ret == ERROR_BUFFER_OVERFLOW) {
>> + g_free(*adptr_addrs);
>> + *adptr_addrs = NULL;
>> + alloc_tries++;
>> + }
>> +
>> + } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries <
> MAX_ALLOC_TRIES));
> Why we do it twice?
>> + if (ret != ERROR_SUCCESS) {
>> + if (*adptr_addrs) {
>> + g_free(*adptr_addrs);
>> + *adptr_addrs = NULL;
>> + }
>> + }
> In this case we always have ERROR_SUCCESS.
>> + return ret;
>> +}
>> +
> The comments for this function is the same. Moreover, they look very
> similar,
> can we merge them and have #if (_WIN32_WINNT < 0x0600) inside the
> function body?
I'll do the same as above, get the size first and do one allocation. Since we
are doing GetAdaptersAddresses and GetAdaptersInfo, I don't see usable way in
combining these two. Simplifying th
e memory allocation should help though.
>> +#if (_WIN32_WINNT < 0x0600)
>> +static DWORD guest_get_adapters_info(IP_ADAPTER_INFO **adptr_info)
>> +{
>> + ULONG out_buf_len = 0;
>> + int alloc_tries = 0;
>> + DWORD ret = ERROR_SUCCESS;
>> +
>> + out_buf_len = sizeof(IP_ADAPTER_INFO);
>> + do {
>> + *adptr_info = (IP_ADAPTER_INFO *)g_malloc(out_buf_len);
>> + if (*adptr_info == NULL) {
>> + return ERROR_NOT_ENOUGH_MEMORY;
>> + }
>> +
>> + ret = GetAdaptersInfo(*adptr_info, &out_buf_len);
>> +
>> + if (ret == ERROR_BUFFER_OVERFLOW) {
>> + g_free(*adptr_info);
>> + *adptr_info = NULL;
>> + alloc_tries++;
>> + }
>> +
>> + } while ((ret == ERROR_BUFFER_OVERFLOW) && (alloc_tries <
> MAX_ALLOC_TRIES));
>> +
>> + if (ret != ERROR_SUCCESS) {
>> + if (*adptr_info) {
>> + g_free(*adptr_info);
>> + *adptr_info = NULL;
>> + }
>> + }
>> + return ret;
>> +}
>> +#endif
>> +static char *guest_wcstombs_dup(WCHAR *wstr)
>> +{
>> + char *str;
>> + int i;
>> +
>> + i = wcslen(wstr) + 1;
>> + str = g_malloc(i);
>> + if (str) {
> This is Windows-specific part of qemu-ga, so we should win32 API use
> WideCharToMultiByte.
> I am not sure that wcstombs uses Unicode UTF-16 and afaik it is deprecated.
Will switch to WideCharToMultiByte.
>> + wcstombs(str, wstr, i);
>> + }
>> + return str;
>> +}
>> +
>> +static char *guest_inet_ntop(int af, void *cp, char *buf, socklen_t len)
>> +{
>> +#if (_WIN32_WINNT >= 0x0600) && defined(ARCH_x86_64)
>> + /* If built for 64 bit Windows Vista/2008 or newer, inet_ntop() is
>> + * available for use. Otherwise, do our best to derive it.
>> + */
>> + return (char *)inet_ntop(af, cp, buf, len);
>> +#else
>> + u_char *p;
>> +
>> + p = (u_char *)cp;
>> + if (af == AF_INET) {
> May be we should do some struct IP_addr that will hold this information
> in appropriate format.
inet_ntop just returns a string, so I'm not sure what else could be done here.
>> + snprintf(buf, len, "%u.%u.%u.%u", p[0], p[1], p[2], p[3]);
>> + } else if (af == AF_INET6) {
>> + int i, compressed_zeros, added_to_buf;
>> + char t[sizeof "::ffff"];
>> +
>> + buf[0] = '\0';
>> + compressed_zeros = 0;
>> + added_to_buf = 0;
>> + for (i = 0; i < 16; i += 2) {
>> + if (p[i] != 0 || p[i + 1] != 0) {
>> + if (compressed_zeros) {
>> + if (len > sizeof "::") {
>> + strcat(buf, "::");
>> + len -= sizeof "::" - 1;
>> + }
>> + added_to_buf++;
>> + } else {
>> + if (added_to_buf) {
>> + if (len > 1) {
>> + strcat(buf, ":");
>> + len--;
>> + }
>> + }
>> + added_to_buf++;
>> + }
>> +
>> + /* Take into account leading zeros. */
>> + if (p[i]) {
>> + len -= snprintf(t, sizeof(t), "%x%02x", p[i], p[i+1]);
>> + } else {
>> + len -= snprintf(t, sizeof(t), "%x", p[i+1]);
>> + }
>> +
>> + /* Ensure there's enough room for the NULL. */
>> + if (len > 0) {
>> + strcat(buf, t);
>> + }
>> + compressed_zeros = 0;
>> + } else {
>> + compressed_zeros++;
>> + }
>> + }
>> + if (compressed_zeros && !added_to_buf) {
>> + /* The address was all zeros. */
>> + strcat(buf, "::");
>> + }
>> + }
>> + return buf;
>> +#endif
>> +}
>> +
>> +static int64_t guest_ip_prefix(IP_ADAPTER_UNICAST_ADDRESS *ip_addr)
>> +{
>> +#if (_WIN32_WINNT >= 0x0600
)
>> + /* If built for Windows Vista/2008 or newer, use the OnLinkPrefixLength
>> + * field to obtain the prefix. Otherwise, do our best to figure it
> out.
>> + */
>> + return ip_addr->OnLinkPrefixLength;
>> +#else
>> + int64_t prefix = -1; /* Use for AF_INET6 and unknown/undetermined
> values. */
>> +
>> + if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
>> + IP_ADAPTER_INFO *adptr_info, *info;
>> + IP_ADDR_STRING *ip;
>> + struct in_addr *p;
>> +
>> + if (guest_get_adapters_info(&adptr_info) != ERROR_SUCCESS) {
>> + return prefix;
>> + }
>> +
>> + /* Match up the passed in ip_addr with one found in adaptr_info.
>> + * The matching one in adptr_info will have the netmask.
>> + */
>> + p = &((struct sockaddr_in *)ip_addr->Address.lpSockaddr)->sin_addr;
>> + for (info = adptr_info; info && prefix == -1; info = info->Next) {
>> + for (ip = &info->IpAddressList; ip; ip = ip->Next) {
>> + if (p->S_un.S_addr == inet_addr(ip->IpAddress.String)) {
>> + prefix = ctpop32(inet_addr(ip->IpMask.String));
>> + break;
>> + }
>> + }
>> + }
>> + g_free(adptr_info);
>> + }
>> + return prefix;
>> +#endif
>> +}
>> +
>> GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
>> {
>> - error_set(errp, QERR_UNSUPPORTED);
>> + IP_ADAPTER_ADDRESSES *adptr_addrs, *addr;
>> + GuestNetworkInterfaceList *head = NULL, *cur_item = NULL;
>> + GuestIpAddressList *head_addr, *cur_addr;
>> + DWORD ret;
>> +
>> + ret = guest_get_adapter_addresses(&adptr_addrs);
>> + if (ret != ERROR_SUCCESS) {
>> + error_setg(errp, "failed to get adapter addresses %lu", ret);
>> + return NULL;
>> + }
>> +
>> + for (addr = adptr_addrs; addr; addr = addr->Next) {
>> + GuestNetworkInterfaceList *info;
>> + GuestIpAddressList *address_item = NULL;
>> + char addr4[INET_ADDRSTRLEN];
>> + char addr6[INET6_ADDRSTRLEN];
>> + unsigned char *mac_addr;
>> + void *p;
> Variables should not be declared in function body. I think better way is
> to put them in the beginning.
I'll move the variable to the beginning.
>> + IP_ADAPTER_UNICAST_ADDRESS *ip_addr = NULL;
>> +
>> + info = g_malloc0(sizeof(*info));
>> + if (!info) {
>> + error_setg(errp, "failed to alloc a network interface list");
>> + goto error;
>> + }
>> +
>> + if (!cur_item) {
>> + head = cur_item = info;
>> + } else {
>> + cur_item->next = info;
>> + cur_item = info;
>> + }
>> +
>> + info->value = g_malloc0(sizeof(*info->value));
>> + if (!info->value) {
>> + error_setg(errp, "failed to alloc a network interface");
>> + goto error;
>> + }
>> + info->value->name = guest_wcstombs_dup(addr->FriendlyName);
>> +
>> + if (addr->PhysicalAddressLength) {
>> + mac_addr = addr->PhysicalAddress;
>> +
>> + info->value->hardware_address =
>> + g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
>> + (int) mac_addr[0], (int) mac_addr[1],
>> + (int) mac_addr[2], (int) mac_addr[3],
>> + (int) mac_addr[4], (int) mac_addr[5]);
>> +
>> + info->value->has_hardware_address = true;
>> + }
>> +
>> + head_addr = NULL;
>> + cur_addr = NULL;
>> + for (ip_addr = addr->FirstUnicastAddress;
>> + ip_addr;
>> + ip_addr = ip_addr->Next) {
>> + address_item = g_malloc0(sizeof(*address_item));
>> + if (!address_item) {
>> + error_setg(errp, "failed to alloc an Ip address list");
>> + goto error;
>> + }
>> +
>> + if (!cur_addr) {
>> +
head_addr = cur_addr = address_item;
>> + } else {
>> + cur_addr->next = address_item;
>> + cur_addr = address_item;
>> + }
>> +
>> + address_item->value = g_malloc0(sizeof(*address_item->value));
>> + if (!address_item->value) {
>> + error_setg(errp, "failed to alloc an Ip address");
>> + goto error;
>> + }
>> +
>> + if (ip_addr->Address.lpSockaddr->sa_family == AF_INET) {
>> + p = &((struct sockaddr_in *)
>> + ip_addr->Address.lpSockaddr)->sin_addr;
>> +
>> + if (!guest_inet_ntop(AF_INET, p, addr4, sizeof(addr4))) {
>> + error_setg(errp,
> win32 error macro
Since the error can be retrieved with WSAGetLastErorr, I'll switch to
error_setg_win32(errp, WSAGetLastError(), ...
>> + "failed address presentation form
> conversion");
>> + goto error;
>> + }
>> +
>> + address_item->value->ip_address = g_strdup(addr4);
>> + address_item->value->ip_address_type =
>> + GUEST_IP_ADDRESS_TYPE_IPV4;
>> + } else if (ip_addr->Address.lpSockaddr->sa_family == AF_INET6) {
>> + p = &((struct sockaddr_in6 *)
>> + ip_addr->Address.lpSockaddr)->sin6_addr;
>> +
>> + if (!guest_inet_ntop(AF_INET6, p, addr6, sizeof(addr6))) {
>> + error_setg(errp,
>> + "failed address presentation form
> conversion");
> win32 error macro
Same as above.
>> goto error;
>> + }
>> +
>> + address_item->value->ip_address = g_strdup(addr6);
>> + address_item->value->ip_address_type =
>> + GUEST_IP_ADDRESS_TYPE_IPV6;
>> + }
>> + address_item->value->prefix = guest_ip_prefix(ip_addr);
>> + }
>> + if (head_addr) {
>> + info->value->has_ip_addresses = true;
>> + info->value->ip_addresses = head_addr;
>> + }
>> + }
>> +
>> + if (adptr_addrs) {
>> + g_free(adptr_addrs);
>> + }
>> + return head;
>> +
>> +error:
>> + if (adptr_addrs) {
>> + g_free(adptr_addrs);
>> + }
>> + if (head) {
>> + qapi_free_GuestNetworkInterfaceList(head);
>> + }
>> return NULL;
>> }
>>
>> @@ -707,7 +1022,7 @@ GuestMemoryBlockInfo
> *qmp_guest_get_memory_block_info(Error **errp)
>> GList *ga_command_blacklist_init(GList *blacklist)
>> {
>> const char *list_unsupported[] = {
>> - "guest-suspend-hybrid", "guest-network-get-interfaces",
>> + "guest-suspend-hybrid",
>> "guest-get-vcpus", "guest-set-vcpus",
>> "guest-set-user-password",
>> "guest-get-memory-blocks", "guest-set-memory-blocks",
> I think 2 last functions GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp) and static char
> *guest_inet_ntop(int af, void *cp, char *buf, socklen_t len)should be
> redesigned and refactored very attentively. Secondly you should decide
> what kind of functions to use Win32 API or POSIX API.
> It is bed idea to mix them.
Where InetNtop is available where inet_ntop is, I'll switch the win32 api.
> Pls, pay attention to to error handling.
For apis that can retrieve the error form GetLastError, I'll switch to the
error_setg_win32.
