[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command |
Date: |
Thu, 23 Feb 2012 12:20:34 -0200 |
On Sun, 19 Feb 2012 12:15:43 +0100
Michal Privoznik <address@hidden> 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.
>
> Signed-off-by: Michal Privoznik <address@hidden>
> ---
> diff to v3:
> -use ctpop32() instead of separate count_one_bits()
>
> diff to v2:
> -Properly set IP addr family for IPv6
>
> diff to v1:
> -move from guest-getip to guest-network-info
> -replace black boxed algorithm for population count
> -several coding styles improvements
> qapi-schema-guest.json | 29 ++++++++
> qga/guest-agent-commands.c | 157
> ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 186 insertions(+), 0 deletions(-)
>
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 5f8a18d..ca4fdc5 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,32 @@
> ##
> { 'command': 'guest-fsfreeze-thaw',
> 'returns': 'int' }
> +
> +##
> +# @guest-network-info:
> +#
> +# Get list of guest IP addresses, MAC addresses
> +# and netmasks.
> +#
> +# @name: The name of interface for which info are being delivered
> +#
> +# @ipaddr: IP address assigned to @name
> +#
> +# @ipaddrtype: Type of @ipaddr (e.g. ipv4, ipv6)
> +#
> +# @prefix: Network prefix length
> +#
> +# @hwaddr: Hardware address of @name
> +#
> +# Returns: List of GuestNetworkInfo on success.
> +#
> +# Since: 1.1
> +##
> +{ 'enum': 'GuestIpAddrType',
> + 'data': [ 'ipv4', 'ipv6' ] }
> +{ 'type': 'GuestNetworkInfo',
> + 'data': {'iface': {'name': 'str', 'ipaddr': 'str',
> + 'ipaddrtype': 'GuestIpAddrType',
> + 'prefix': 'int', 'hwaddr': 'str'} } }
> +{ 'command': 'guest-network-info',
> + 'returns': ['GuestNetworkInfo'] }
No need for short names like 'ipaddr', longer names like 'ip-address' are
better.
Also, please, document the enum, the type and the command separately. You can
look
for examples in the qapi-schema.json file (yes, we're not doing it in this
file, but
we should).
Do we really need the 'iface' dict, btw?
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..1371f8b 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -5,6 +5,7 @@
> *
> * Authors:
> * Michael Roth <address@hidden>
> + * Michal Privoznik <address@hidden>
> *
> * 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,10 +24,15 @@
>
> #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"
> #include "qemu-queue.h"
> +#include "host-utils.h"
>
> static GAState *ga_state;
>
> @@ -583,3 +589,154 @@ void ga_command_state_init(GAState *s, GACommandState
> *cs)
> #endif
> ga_command_state_add(cs, guest_file_init, NULL);
> }
> +
> +/*
> + * Get the list of interfaces among with their
> + * IP/MAC addresses, prefixes and IP versions
> + */
> +GuestNetworkInfoList *qmp_guest_network_info(Error **err)
> +{
> + GuestNetworkInfoList *head = NULL, *cur_item = NULL;
> + struct ifaddrs *ifap = NULL, *ifa = NULL;
> + char err_msg[512];
> +
> + g_debug("guest-network-info 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 error;
> + }
Generic errors are bad for the user. The best thing to do would be to have a
QERR_
for the possible errors getifaddrs() can return. However, getifaddrs() can
return
errors from several functions, so I don't know what's the best thing to do here.
Ideas, Michael?
> +
> + ifa = ifap;
> + while (ifa) {
> + GuestNetworkInfoList *info = NULL;
> + char addr4[INET_ADDRSTRLEN];
> + char addr6[INET6_ADDRSTRLEN];
> + unsigned char *mac_addr;
> + void *tmp_addr_ptr = NULL;
I'd call this just *p.
> + int sock, family;
> + int mac_supported;
> + struct ifreq ifr;
> +
> + /* Step over interfaces without an address */
> + if (!ifa->ifa_addr) {
> + ifa = ifa->ifa_next;
> + continue;
> + }
This matches the documentation, but wouldn't it be better to return information
about _interfaces_ (vs. _ip addresses_)? We could make the ip-address field
optional then.
> +
> + g_debug("Processing %s interface", ifa->ifa_name);
> +
> + family = ifa->ifa_addr->sa_family;
> +
> + if (family == AF_INET) {
> + /* interface with IPv4 address */
> + tmp_addr_ptr = &((struct sockaddr_in *)ifa->ifa_addr)->sin_addr;
> + inet_ntop(AF_INET, tmp_addr_ptr, addr4, sizeof(addr4));
inet_ntop() can fail.
> +
> + info = g_malloc0(sizeof(*info));
> + info->value = g_malloc0(sizeof(*info->value));
> + info->value->iface.name = g_strdup(ifa->ifa_name);
The three lines above can be moved outside the if else block.
> + info->value->iface.ipaddr = g_strdup(addr4);
> + info->value->iface.ipaddrtype = GUEST_IP_ADDR_TYPE_IPV4;
> +
> + /* Count the number of set bits in netmask.
> + * This is safe as '1' and '0' cannot be shuffled in netmask. */
> + tmp_addr_ptr = &((struct sockaddr_in
> *)ifa->ifa_netmask)->sin_addr;
> + info->value->iface.prefix =
> + ctpop32(((uint32_t *) tmp_addr_ptr)[0]);
> + } else if (family == AF_INET6) {
> + /* interface with IPv6 address */
> + tmp_addr_ptr = &((struct sockaddr_in6
> *)ifa->ifa_addr)->sin6_addr;
> + inet_ntop(AF_INET6, tmp_addr_ptr, 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);
> + info->value->iface.ipaddrtype = GUEST_IP_ADDR_TYPE_IPV6;
> +
> + /* Count the number of set bits in netmask.
> + * This is safe as '1' and '0' cannot be shuffled in netmask. */
> + tmp_addr_ptr = &((struct sockaddr_in6
> *)ifa->ifa_netmask)->sin6_addr;
> + info->value->iface.prefix =
> + ctpop32(((uint32_t *) tmp_addr_ptr)[0]) +
> + ctpop32(((uint32_t *) tmp_addr_ptr)[1]) +
> + ctpop32(((uint32_t *) tmp_addr_ptr)[2]) +
> + ctpop32(((uint32_t *) tmp_addr_ptr)[3]);
> + }
> +
> + /* Add new family here */
The comment is not needed.
> +
> + mac_supported = ifa->ifa_flags & SIOCGIFHWADDR;
> +
> + ifa = ifa->ifa_next;
> +
> + if (!info) {
> + continue;
> + }
How can info be NULL here?
> +
> + if (!cur_item) {
> + head = cur_item = info;
> + } else {
> + cur_item->next = info;
> + cur_item = info;
> + }
> +
> + 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;
> + }
Is 'mac_supported' really needed? I'd just do 'ifa->ifa_flags & SIOCGIFHWADDR'.
> +
> + 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 error;
> + }
Generic error.
> +
> + 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 error;
> + }
Generic error.
> +
> + 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 error;
> + }
> +
> + close(sock);
> + }
> +
> + freeifaddrs(ifap);
> + return head;
> +
> +error:
> + freeifaddrs(ifap);
> + qapi_free_GuestNetworkInfoList(head);
> + return NULL;
> +}
- [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Michal Privoznik, 2012/02/19
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Michal Privoznik, 2012/02/22
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command,
Luiz Capitulino <=
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Michal Privoznik, 2012/02/27
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Michael Roth, 2012/02/27
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Luiz Capitulino, 2012/02/28
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Michael Roth, 2012/02/28
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Luiz Capitulino, 2012/02/28
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Michal Privoznik, 2012/02/29
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Luiz Capitulino, 2012/02/29
- Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command, Michael Roth, 2012/02/29