Re: [Qemu-devel] [PATCH v4] qemu-ga: Add guest-network-info command

[Michael Roth]

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

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
>