[v4] qemu-ga: Add guest-network-info command

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>
---

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?

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

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

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
>

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.

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.
>

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.

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?

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 :)

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.

>