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

[Michal Privoznik <mprivozn@redhat.com>]

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