All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
@ 2012-02-19 11:15 Michal Privoznik
  2012-02-22 17:10 ` Michal Privoznik
  2012-02-23 14:20 ` Luiz Capitulino
  0 siblings, 2 replies; 11+ messages in thread
From: Michal Privoznik @ 2012-02-19 11:15 UTC (permalink / raw)
  To: qemu-devel

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 <mprivozn@redhat.com>
---
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'] }
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      <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,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;
+    }
+
+    ifa = ifap;
+    while (ifa) {
+        GuestNetworkInfoList *info = NULL;
+        char addr4[INET_ADDRSTRLEN];
+        char addr6[INET6_ADDRSTRLEN];
+        unsigned char *mac_addr;
+        void *tmp_addr_ptr = NULL;
+        int sock, family;
+        int mac_supported;
+        struct ifreq ifr;
+
+        /* Step over interfaces without an address */
+        if (!ifa->ifa_addr) {
+            ifa = ifa->ifa_next;
+            continue;
+        }
+
+        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));
+
+            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);
+            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 */
+
+        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;
+        }
+
+        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 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;
+        }
+
+        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;
+}
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-19 11:15 [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command Michal Privoznik
@ 2012-02-22 17:10 ` Michal Privoznik
  2012-02-23 14:20 ` Luiz Capitulino
  1 sibling, 0 replies; 11+ messages in thread
From: Michal Privoznik @ 2012-02-22 17:10 UTC (permalink / raw)
  To: qemu-devel

On 19.02.2012 12:15, 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.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 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(-)
> 

ping?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-19 11:15 [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command Michal Privoznik
  2012-02-22 17:10 ` Michal Privoznik
@ 2012-02-23 14:20 ` Luiz Capitulino
  2012-02-27 18:49   ` Michal Privoznik
  1 sibling, 1 reply; 11+ messages in thread
From: Luiz Capitulino @ 2012-02-23 14:20 UTC (permalink / raw)
  To: Michal Privoznik; +Cc: qemu-devel

On Sun, 19 Feb 2012 12:15:43 +0100
Michal Privoznik <mprivozn@redhat.com> 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 <mprivozn@redhat.com>
> ---
> 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      <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,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;
> +}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-23 14:20 ` Luiz Capitulino
@ 2012-02-27 18:49   ` Michal Privoznik
  2012-02-28  2:07     ` Michael Roth
  0 siblings, 1 reply; 11+ messages in thread
From: Michal Privoznik @ 2012-02-27 18:49 UTC (permalink / raw)
  To: Luiz Capitulino; +Cc: qemu-devel, Michael Roth

On 23.02.2012 15:20, Luiz Capitulino wrote:
> On Sun, 19 Feb 2012 12:15:43 +0100
> Michal Privoznik <mprivozn@redhat.com> 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 <mprivozn@redhat.com>
>> ---
>> 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?

I think yes as it creates an envelope over one quintuplet so it is
obvious what value belongs to what interface. Moreover, if we would ever
expand this command to report a network but not (directly) interface
related info, say routing, whatever, we can simply add another
dictionary here. However, thinking about whole format now, what about
switching to more flexible schema:

{ 'enum': 'GuestIpAddressType',
  'data': [ 'ipv4', 'ipv6' ] }

{ 'type': 'GuestIpAddress',
  'data': {'ip-address': 'str',
           'ip-address-type': 'GuestIpAddressType',
           'prefix': 'int'} }

{ 'type': 'GuestNetworkInfo',
  'data': {'interface': {'name': 'str',
                         '*hardware-address': 'str',
                         '*address': ['GuestIpAddress'] } } }

{ 'command': 'guest-network-info',
  'returns': ['GuestNetworkInfo'] }

The advantage is, one get pair <interface; [list of addresses]> instead
of list<interface; address>; In other words, create an associative array
with network interface name as the key.

>> +/*
>> + * 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?

Once we settle down on this I can send another version for review.
Personally, if guest agent would report description (see my other e-mail
[1]) I don't see big advantage in introducing dozens of error codes here.

1: http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg03171.html

Regards,

Michal

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-27 18:49   ` Michal Privoznik
@ 2012-02-28  2:07     ` Michael Roth
  2012-02-28 14:22       ` Luiz Capitulino
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Roth @ 2012-02-28  2:07 UTC (permalink / raw)
  To: Michal Privoznik; +Cc: qemu-devel, Luiz Capitulino

On Mon, Feb 27, 2012 at 07:49:24PM +0100, Michal Privoznik wrote:
> On 23.02.2012 15:20, Luiz Capitulino wrote:
> > On Sun, 19 Feb 2012 12:15:43 +0100
> > Michal Privoznik <mprivozn@redhat.com> 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 <mprivozn@redhat.com>
> >> ---
> >> 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).

FYI, the documentation changes are upstream now, along with some
restructuring to better co-exist with the windows support. You'll want to
rebase on that: this RPC implementation would go in qga/commands-posix.c now,
and you'll need to add a 'not implemented' stub in qga/commands-win32.c (or
implement it).

> > 
> > Do we really need the 'iface' dict, btw?
> 
> I think yes as it creates an envelope over one quintuplet so it is
> obvious what value belongs to what interface. Moreover, if we would ever
> expand this command to report a network but not (directly) interface
> related info, say routing, whatever, we can simply add another
> dictionary here. However, thinking about whole format now, what about
> switching to more flexible schema:
> 
> { 'enum': 'GuestIpAddressType',
>   'data': [ 'ipv4', 'ipv6' ] }
> 
> { 'type': 'GuestIpAddress',
>   'data': {'ip-address': 'str',
>            'ip-address-type': 'GuestIpAddressType',
>            'prefix': 'int'} }
> 
> { 'type': 'GuestNetworkInfo',
>   'data': {'interface': {'name': 'str',
>                          '*hardware-address': 'str',
>                          '*address': ['GuestIpAddress'] } } }

'*ip-addresses' would be more consistent.

> 
> { 'command': 'guest-network-info',
>   'returns': ['GuestNetworkInfo'] }
> 
> The advantage is, one get pair <interface; [list of addresses]> instead
> of list<interface; address>; In other words, create an associative array
> with network interface name as the key.
> 

I think this approach makes more sense, but I also agree with your statement
above that we may at some point add fields that wouldn't make sense to group
under interfaces, such as routes, bridges, etc.

We *could*, which I think you're suggesting, add fields to GuestNetworkInfo,
and basically make it a union type by having all the top-level fields optional.
Come to think of it, I think Anthony had support for unions in his experimental
branch... but, either way, since you'd have to probe each element in the
response list to figure out the type, I think that might get unwieldly as new
possible fields were added.

What about something like this instead:

{ 'enum': 'GuestIpAddressType',
  'data': [ 'ipv4', 'ipv6' ] }

{ 'type': 'GuestIpAddress',
  'data': {'ip-address': 'str',
           'ip-address-type': 'GuestIpAddressType',
           'prefix': 'int'} }

{ 'type': 'GuestNetworkInterface',
  'data': {'interface': {'name': 'str',
                         '*hardware-address': 'str',
                         '*ip-addresses': ['GuestIpAddress'] } } }

{ 'type': 'GuestNetworkInfo',
  'data': { 'interfaces': ['GuestNetworkInterfaces'] } }

{ 'command': 'guest-network-info',
  'returns': 'GuestNetworkInfo' }

In the future we might have:

{ 'type': 'GuestNetworkInfo',
  'data': { 'interfaces': ['GuestNetworkInterfaces'],
            'routes': ['GuestNetworkRoute'],
            'bridges': ['GuestNetworkBridge'],
            'firewall-rules': ['firewall-rule'], # yikes
            etc. } }

If we only care about interfaces we just iterate through interfaces, no
need to filter through what might be a large list of all sorts of stuff.

> >> +/*
> >> + * 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?
> 
> Once we settle down on this I can send another version for review.
> Personally, if guest agent would report description (see my other e-mail
> [1]) I don't see big advantage in introducing dozens of error codes here.

descriptions are mapped to QERRs though, so it'd only be useful if you
defined specific errors for these cases. I agree with Luiz, but at the
same time it's not exactly tractable to enumerate all possible errors for every
command into a unique QERR except for common things like FD_NOT_FOUND. So maybe
just a QERR_QGA_INTERFACE_ENUMERATION_FAILED, that took a stringified error
message? I don't really have a strong opinion either way.

Didn't see the original message about the "desc" field, but it's pretty
straight-forward if you'd like to send a patch. Basically just, everywhere we'd
set the 'error' field in the response to error_get_qobject(err), you'd
first take that qobject and add the "desc" field via something like:

QObject *error_obj;

error_obj = qdict_put_obj(qobject_to_qdict(error_get_qobject(err)),
                          "desc", error_get_pretty(err));

I think there are some QGA definitions in qerror.h that you'll need to
add to the error description table in qerror.c as well. We set it all
over the place though... might be nicer to add a common interface to qerror.c
or error.c or something, but I'm not sure it's a good idea to
intermingle those error frameworks too much.

> 
> 1: http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg03171.html
> 
> Regards,
> 
> Michal
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-28  2:07     ` Michael Roth
@ 2012-02-28 14:22       ` Luiz Capitulino
  2012-02-28 17:09         ` Michael Roth
  0 siblings, 1 reply; 11+ messages in thread
From: Luiz Capitulino @ 2012-02-28 14:22 UTC (permalink / raw)
  To: Michael Roth; +Cc: Michal Privoznik, qemu-devel

On Mon, 27 Feb 2012 20:07:28 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> What about something like this instead:
> 
> { 'enum': 'GuestIpAddressType',
>   'data': [ 'ipv4', 'ipv6' ] }
> 
> { 'type': 'GuestIpAddress',
>   'data': {'ip-address': 'str',
>            'ip-address-type': 'GuestIpAddressType',
>            'prefix': 'int'} }
> 
> { 'type': 'GuestNetworkInterface',
>   'data': {'interface': {'name': 'str',
>                          '*hardware-address': 'str',
>                          '*ip-addresses': ['GuestIpAddress'] } } }
> 
> { 'type': 'GuestNetworkInfo',
>   'data': { 'interfaces': ['GuestNetworkInterfaces'] } }
> 
> { 'command': 'guest-network-info',
>   'returns': 'GuestNetworkInfo' }
> 
> In the future we might have:
> 
> { 'type': 'GuestNetworkInfo',
>   'data': { 'interfaces': ['GuestNetworkInterfaces'],
>             'routes': ['GuestNetworkRoute'],
>             'bridges': ['GuestNetworkBridge'],
>             'firewall-rules': ['firewall-rule'], # yikes
>             etc. } }

Both approaches are fine to me, but another possibility is to split this into
multiple commands, like guest-interfaces-info, guest-routes-info etc. This would
allow for simpler commands with less clutter.

> > Once we settle down on this I can send another version for review.
> > Personally, if guest agent would report description (see my other e-mail
> > [1]) I don't see big advantage in introducing dozens of error codes here.
> 
> descriptions are mapped to QERRs though, so it'd only be useful if you
> defined specific errors for these cases. I agree with Luiz, but at the
> same time it's not exactly tractable to enumerate all possible errors for every
> command into a unique QERR except for common things like FD_NOT_FOUND. So maybe
> just a QERR_QGA_INTERFACE_ENUMERATION_FAILED, that took a stringified error
> message? I don't really have a strong opinion either way.

Well, turns out I'm not sure what to do here either. On the one hand it's a
huge work (and probably unnecessary) to add all possible errors. On the other
hand, it's really hard to debug a problem when all information you have is
a generic error.

As this a relatively simple query command, I'm fine with simple/generic errors.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-28 14:22       ` Luiz Capitulino
@ 2012-02-28 17:09         ` Michael Roth
  2012-02-28 17:41           ` Luiz Capitulino
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Roth @ 2012-02-28 17:09 UTC (permalink / raw)
  To: Luiz Capitulino; +Cc: Michal Privoznik, qemu-devel

On Tue, Feb 28, 2012 at 11:22:22AM -0300, Luiz Capitulino wrote:
> On Mon, 27 Feb 2012 20:07:28 -0600
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > What about something like this instead:
> > 
> > { 'enum': 'GuestIpAddressType',
> >   'data': [ 'ipv4', 'ipv6' ] }
> > 
> > { 'type': 'GuestIpAddress',
> >   'data': {'ip-address': 'str',
> >            'ip-address-type': 'GuestIpAddressType',
> >            'prefix': 'int'} }
> > 
> > { 'type': 'GuestNetworkInterface',
> >   'data': {'interface': {'name': 'str',
> >                          '*hardware-address': 'str',
> >                          '*ip-addresses': ['GuestIpAddress'] } } }
> > 
> > { 'type': 'GuestNetworkInfo',
> >   'data': { 'interfaces': ['GuestNetworkInterfaces'] } }
> > 
> > { 'command': 'guest-network-info',
> >   'returns': 'GuestNetworkInfo' }
> > 
> > In the future we might have:
> > 
> > { 'type': 'GuestNetworkInfo',
> >   'data': { 'interfaces': ['GuestNetworkInterfaces'],
> >             'routes': ['GuestNetworkRoute'],
> >             'bridges': ['GuestNetworkBridge'],
> >             'firewall-rules': ['firewall-rule'], # yikes
> >             etc. } }
> 
> Both approaches are fine to me, but another possibility is to split this into
> multiple commands, like guest-interfaces-info, guest-routes-info etc. This would
> allow for simpler commands with less clutter.

Hmm, I know Michal already sent a new version with my suggestions, but
you're right, splitting out the commands simplified both the responses,
and makes it easier to discover whether or not that information is
available, since you can look for the command in guest-info before
attempting it, rather than attempting it and then looking at the result.

So maybe just something this?:

{ 'type': 'GuestNetworkInterface',
  'data': { 'name': 'str',
            '*hardware-address': 'str',
            '*ip-addresses': ['GuestIpAddress'] } } }

{ 'command': 'guest-network-interfaces',
  'returns': ['GuestNetworkInterface'] }

Michal, does this seem reasonable to you?

> 
> > > Once we settle down on this I can send another version for review.
> > > Personally, if guest agent would report description (see my other e-mail
> > > [1]) I don't see big advantage in introducing dozens of error codes here.
> > 
> > descriptions are mapped to QERRs though, so it'd only be useful if you
> > defined specific errors for these cases. I agree with Luiz, but at the
> > same time it's not exactly tractable to enumerate all possible errors for every
> > command into a unique QERR except for common things like FD_NOT_FOUND. So maybe
> > just a QERR_QGA_INTERFACE_ENUMERATION_FAILED, that took a stringified error
> > message? I don't really have a strong opinion either way.
> 
> Well, turns out I'm not sure what to do here either. On the one hand it's a
> huge work (and probably unnecessary) to add all possible errors. On the other
> hand, it's really hard to debug a problem when all information you have is
> a generic error.
> 
> As this a relatively simple query command, I'm fine with simple/generic errors.
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-28 17:09         ` Michael Roth
@ 2012-02-28 17:41           ` Luiz Capitulino
  2012-02-29 13:17             ` Michal Privoznik
  0 siblings, 1 reply; 11+ messages in thread
From: Luiz Capitulino @ 2012-02-28 17:41 UTC (permalink / raw)
  To: Michael Roth; +Cc: Michal Privoznik, qemu-devel

On Tue, 28 Feb 2012 11:09:38 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Tue, Feb 28, 2012 at 11:22:22AM -0300, Luiz Capitulino wrote:
> > On Mon, 27 Feb 2012 20:07:28 -0600
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > What about something like this instead:
> > > 
> > > { 'enum': 'GuestIpAddressType',
> > >   'data': [ 'ipv4', 'ipv6' ] }
> > > 
> > > { 'type': 'GuestIpAddress',
> > >   'data': {'ip-address': 'str',
> > >            'ip-address-type': 'GuestIpAddressType',
> > >            'prefix': 'int'} }
> > > 
> > > { 'type': 'GuestNetworkInterface',
> > >   'data': {'interface': {'name': 'str',
> > >                          '*hardware-address': 'str',
> > >                          '*ip-addresses': ['GuestIpAddress'] } } }
> > > 
> > > { 'type': 'GuestNetworkInfo',
> > >   'data': { 'interfaces': ['GuestNetworkInterfaces'] } }
> > > 
> > > { 'command': 'guest-network-info',
> > >   'returns': 'GuestNetworkInfo' }
> > > 
> > > In the future we might have:
> > > 
> > > { 'type': 'GuestNetworkInfo',
> > >   'data': { 'interfaces': ['GuestNetworkInterfaces'],
> > >             'routes': ['GuestNetworkRoute'],
> > >             'bridges': ['GuestNetworkBridge'],
> > >             'firewall-rules': ['firewall-rule'], # yikes
> > >             etc. } }
> > 
> > Both approaches are fine to me, but another possibility is to split this into
> > multiple commands, like guest-interfaces-info, guest-routes-info etc. This would
> > allow for simpler commands with less clutter.
> 
> Hmm, I know Michal already sent a new version with my suggestions, but
> you're right, splitting out the commands simplified both the responses,
> and makes it easier to discover whether or not that information is
> available, since you can look for the command in guest-info before
> attempting it, rather than attempting it and then looking at the result.
> 
> So maybe just something this?:
> 
> { 'type': 'GuestNetworkInterface',
>   'data': { 'name': 'str',
>             '*hardware-address': 'str',
>             '*ip-addresses': ['GuestIpAddress'] } } }
> 
> { 'command': 'guest-network-interfaces',
>   'returns': ['GuestNetworkInterface'] }

Looks good to me, the only nitpick is that I think command names should be
verbs.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-28 17:41           ` Luiz Capitulino
@ 2012-02-29 13:17             ` Michal Privoznik
  2012-02-29 15:01               ` Luiz Capitulino
  0 siblings, 1 reply; 11+ messages in thread
From: Michal Privoznik @ 2012-02-29 13:17 UTC (permalink / raw)
  To: Luiz Capitulino; +Cc: Michael Roth, qemu-devel

On 28.02.2012 18:41, Luiz Capitulino wrote:
>> Hmm, I know Michal already sent a new version with my suggestions, but
>> > you're right, splitting out the commands simplified both the responses,
>> > and makes it easier to discover whether or not that information is
>> > available, since you can look for the command in guest-info before
>> > attempting it, rather than attempting it and then looking at the result.
>> > 
>> > So maybe just something this?:
>> > 
>> > { 'type': 'GuestNetworkInterface',
>> >   'data': { 'name': 'str',
>> >             '*hardware-address': 'str',
>> >             '*ip-addresses': ['GuestIpAddress'] } } }
>> > 
>> > { 'command': 'guest-network-interfaces',
>> >   'returns': ['GuestNetworkInterface'] }
> Looks good to me, the only nitpick is that I think command names should be
> verbs.

guest-get-network-interfaces?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-29 13:17             ` Michal Privoznik
@ 2012-02-29 15:01               ` Luiz Capitulino
  2012-02-29 15:32                 ` Michael Roth
  0 siblings, 1 reply; 11+ messages in thread
From: Luiz Capitulino @ 2012-02-29 15:01 UTC (permalink / raw)
  To: Michal Privoznik; +Cc: Michael Roth, qemu-devel

On Wed, 29 Feb 2012 14:17:52 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 28.02.2012 18:41, Luiz Capitulino wrote:
> >> Hmm, I know Michal already sent a new version with my suggestions, but
> >> > you're right, splitting out the commands simplified both the responses,
> >> > and makes it easier to discover whether or not that information is
> >> > available, since you can look for the command in guest-info before
> >> > attempting it, rather than attempting it and then looking at the result.
> >> > 
> >> > So maybe just something this?:
> >> > 
> >> > { 'type': 'GuestNetworkInterface',
> >> >   'data': { 'name': 'str',
> >> >             '*hardware-address': 'str',
> >> >             '*ip-addresses': ['GuestIpAddress'] } } }
> >> > 
> >> > { 'command': 'guest-network-interfaces',
> >> >   'returns': ['GuestNetworkInterface'] }
> > Looks good to me, the only nitpick is that I think command names should be
> > verbs.
> 
> guest-get-network-interfaces?

Works for me, but would be good to agree on a standard for it. I'll defer
the decision to Michael, as he's the actual maintainer :)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command
  2012-02-29 15:01               ` Luiz Capitulino
@ 2012-02-29 15:32                 ` Michael Roth
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Roth @ 2012-02-29 15:32 UTC (permalink / raw)
  To: Luiz Capitulino; +Cc: Michal Privoznik, qemu-devel

On Wed, Feb 29, 2012 at 12:01:16PM -0300, Luiz Capitulino wrote:
> On Wed, 29 Feb 2012 14:17:52 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
> > On 28.02.2012 18:41, Luiz Capitulino wrote:
> > >> Hmm, I know Michal already sent a new version with my suggestions, but
> > >> > you're right, splitting out the commands simplified both the responses,
> > >> > and makes it easier to discover whether or not that information is
> > >> > available, since you can look for the command in guest-info before
> > >> > attempting it, rather than attempting it and then looking at the result.
> > >> > 
> > >> > So maybe just something this?:
> > >> > 
> > >> > { 'type': 'GuestNetworkInterface',
> > >> >   'data': { 'name': 'str',
> > >> >             '*hardware-address': 'str',
> > >> >             '*ip-addresses': ['GuestIpAddress'] } } }
> > >> > 
> > >> > { 'command': 'guest-network-interfaces',
> > >> >   'returns': ['GuestNetworkInterface'] }
> > > Looks good to me, the only nitpick is that I think command names should be
> > > verbs.
> > 
> > guest-get-network-interfaces?
> 
> Works for me, but would be good to agree on a standard for it. I'll defer
> the decision to Michael, as he's the actual maintainer :)

I would prefer "guest-network-get-interfaces" since it's more in keeping
with the guest-file-* and guest-fsfreeze-* commands, but other than
that, looks good.

> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-02-29 15:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-19 11:15 [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command Michal Privoznik
2012-02-22 17:10 ` Michal Privoznik
2012-02-23 14:20 ` Luiz Capitulino
2012-02-27 18:49   ` Michal Privoznik
2012-02-28  2:07     ` Michael Roth
2012-02-28 14:22       ` Luiz Capitulino
2012-02-28 17:09         ` Michael Roth
2012-02-28 17:41           ` Luiz Capitulino
2012-02-29 13:17             ` Michal Privoznik
2012-02-29 15:01               ` Luiz Capitulino
2012-02-29 15:32                 ` Michael Roth

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.