Re: [Qemu-devel] [PATCH] qemu-ga: Add guest-getip command

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Thu, Feb 16, 2012 at 06:25:03PM +0100, Michal Privoznik wrote:
> This command returns an array of:
> 
>  [ifname, ipaddr, ipaddr_family, prefix, hwaddr]
> 
> for each interface in the system that has an IP address.
> Currently, only IPv4 and IPv6 are supported.

Cool stuff, seems pretty useful. Some comments below:

> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  qapi-schema-guest.json     |   16 +++++
>  qga/guest-agent-commands.c |  156 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 172 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 5f8a18d..14de133 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,19 @@
>  ##
>  { 'command': 'guest-fsfreeze-thaw',
>    'returns': 'int' }
> +
> +##
> +# @guest-getip:

I'd prefer guest-get-ip. But furthermore, we're returning more than just
IPs so maybe something like guest-network-info? It also fits the guest-file-*
and guest-fsfreeze-* format better, which is nice if we ever add new
guest-network-* commands.

> +#
> +# Get list of guest IP addresses, MAC address

*addresses

> +# and netmasks
> +#
> +# Returns: List of GuestIFAddress
> +#
> +# Since: 1.1
> +##
> +{ 'type': 'GuestIFAddress',
> +  'data': {'iface': {'name': 'str', 'ipaddr': 'str', 'ipaddrtype': 'int',

ipaddrtype seems like a good candidate for an enum type, this lets us map to
something less OS-specific and more user-readable, like "ipv4" and "ipv6".
See: GuestFsfreezeStatus.

> +                     'prefix': 'int', 'hwaddr': 'str'} } }

It's not upstream yet, but we've moved to documenting all user-defined types
in the same manner we currently do with qapi-schema.json, so let's
please do that here as well.

> +{ 'command': 'guest-getip',
> +  'returns': ['GuestIFAddress'] }
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..4caffa7 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -5,6 +5,7 @@
>   *
>   * Authors:
>   *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *  Michal Privoznik  <mprivozn@redhat.com>
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -23,6 +24,10 @@
> 
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
> +#include <ifaddrs.h>
> +#include <arpa/inet.h>
> +#include <sys/socket.h>
> +#include <net/if.h>
>  #include "qga/guest-agent-core.h"
>  #include "qga-qmp-commands.h"
>  #include "qerror.h"
> @@ -583,3 +588,154 @@ void ga_command_state_init(GAState *s, GACommandState *cs)
>  #endif
>      ga_command_state_add(cs, guest_file_init, NULL);
>  }
> +
> +/* Count the number of '1' bits in @x */
> +static int32_t
> +count_one_bits(uint32_t x)
> +{
> +  x = ((x & 0xaaaaaaaaU) >> 1) + (x & 0x55555555U);
> +  x = ((x & 0xccccccccU) >> 2) + (x & 0x33333333U);
> +  x = (x >> 16) + (x & 0xffff);
> +  x = ((x & 0xf0f0) >> 4) + (x & 0x0f0f);
> +  return (x >> 8) + (x & 0x00ff);
> +}

I think my brain is too small for this. Why not just:

for (i = 0, count = 0; i < 32; i++) {
    if (x & (1 << i)) {
        count++;
    }
}
return count;

?

> +
> +/*
> + * Get the list of interfaces among with their
> + * IP/MAC addresses, prefixes and IP versions
> + */
> +GuestIFAddressList * qmp_guest_getip(Error **err)
> +{
> +    GuestIFAddressList *head = NULL, *cur_item = NULL;
> +    struct ifaddrs *ifap = NULL, *ifa = NULL;
> +    char err_msg[512];
> +
> +    slog("guest-getip called");
> +
> +    if (getifaddrs(&ifap) < 0) {
> +        snprintf(err_msg, sizeof(err_msg),
> +                 "getifaddrs failed : %s", strerror(errno));
> +        error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +        goto cleanup;
> +    }
> +
> +    ifa = ifap;
> +    while (ifa) {
> +        GuestIFAddressList *info = NULL;
> +        char addr4[INET_ADDRSTRLEN];
> +        char addr6[INET6_ADDRSTRLEN];
> +        unsigned char *mac_addr;
> +        void *tmpAddrPtr = NULL;

*tmp_addr_ptr. Avoid camelcase for non-structured types (QEMU coding
guidelines).

> +        int sock, family;
> +        int mac_supported;
> +        struct ifreq ifr;
> +
> +        /* Step over interfaces without an address */
> +        if (!ifa->ifa_addr)
> +            continue;
> +
> +        g_debug("Processing %s interface", ifa->ifa_name);
> +
> +        family = ifa->ifa_addr->sa_family;
> +
> +        if (family == AF_INET) {
> +            /* interface with IPv4 address */
> +            tmpAddrPtr = &((struct sockaddr_in *)ifa->ifa_addr)->sin_addr;
> +            inet_ntop(AF_INET, tmpAddrPtr, addr4, sizeof(addr4));
> +
> +            info = g_malloc0(sizeof(*info));
> +            info->value = g_malloc0(sizeof(*info->value));
> +            info->value->iface.name = g_strdup(ifa->ifa_name);
> +            info->value->iface.ipaddr = g_strdup(addr4);
> +
> +            /* This is safe as '1' and '0' cannot be shuffled in netmask. */
> +            tmpAddrPtr = &((struct sockaddr_in *)ifa->ifa_netmask)->sin_addr;
> +            info->value->iface.prefix = count_one_bits(((uint32_t *) tmpAddrPtr)[0]);
> +        } else if (family == AF_INET6) {
> +            /* interface with IPv6 address */
> +            tmpAddrPtr = &((struct sockaddr_in6 *)ifa->ifa_addr)->sin6_addr;
> +            inet_ntop(AF_INET6, tmpAddrPtr, addr6, sizeof(addr6));
> +
> +            info = g_malloc0(sizeof(*info));
> +            info->value = g_malloc0(sizeof(*info->value));
> +            info->value->iface.name = g_strdup(ifa->ifa_name);
> +            info->value->iface.ipaddr = g_strdup(addr6);
> +
> +            /* This is safe as '1' and '0' cannot be shuffled in netmask. */
> +            tmpAddrPtr=&((struct sockaddr_in6 *)ifa->ifa_netmask)->sin6_addr;
> +            info->value->iface.prefix = count_one_bits(((uint32_t *) tmpAddrPtr)[0]) +
> +                count_one_bits(((uint32_t *) tmpAddrPtr)[1]) +
> +                count_one_bits(((uint32_t *) tmpAddrPtr)[2]) +
> +                count_one_bits(((uint32_t *) tmpAddrPtr)[3]);
> +        }
> +
> +        /* Add new family here */
> +
> +        mac_supported = ifa->ifa_flags & SIOCGIFHWADDR;
> +
> +        ifa = ifa->ifa_next;
> +
> +        if (!info)
> +            continue;
> +
> +        if (!cur_item) {
> +            head = cur_item = info;
> +        } else {
> +            cur_item->next = info;
> +            cur_item = info;
> +        }
> +
> +        info->value->iface.ipaddrtype = family;
> +
> +        g_debug("Appending to return: iface=%s, ip=%s, type=%llu,  prefix=%llu",
> +                info->value->iface.name,
> +                info->value->iface.ipaddr,
> +                (unsigned long long) info->value->iface.ipaddrtype,
> +                (unsigned long long) info->value->iface.prefix);
> +
> +        /* If we can't get get MAC address on this interface,
> +         * don't even try but continue to another interface */
> +        if (!mac_supported)
> +            continue;
> +
> +        sock = socket(PF_INET, SOCK_STREAM, 0);
> +
> +        if (sock == -1) {
> +            snprintf(err_msg, sizeof(err_msg),
> +                     "failed to create socket: %s", strerror(errno));
> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +            goto cleanup;
> +        }
> +
> +        memset(&ifr, 0, sizeof(ifr));
> +        strncpy(ifr.ifr_name,  info->value->iface.name, IF_NAMESIZE);
> +        if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
> +            snprintf(err_msg, sizeof(err_msg),
> +                     "failed to get MAC addres of %s: %s",
> +                     info->value->iface.name,
> +                     strerror(errno));
> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +            goto cleanup;

If we propagate an error we should return NULL and make sure head is
cleaned up (use qapi_free_GuestIFAddressList())

> +        }
> +
> +        mac_addr = (unsigned char *) &ifr.ifr_hwaddr.sa_data;
> +
> +        if (asprintf(&info->value->iface.hwaddr,
> +                     "%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]) == -1) {
> +            snprintf(err_msg, sizeof(err_msg),
> +                     "failed to format MAC: %s", strerror(errno));
> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +            goto cleanup;
> +        }
> +
> +        close(sock);
> +    }
> +
> +cleanup:
> +    freeifaddrs(ifap);
> +
> +    return head;
> +}
> -- 
> 1.7.3.4
> 
>