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

2012-02-29 Thread Michal Privoznik 
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?

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

2012-02-29 Thread Luiz Capitulino 
On Wed, 29 Feb 2012 14:17:52 +0100
Michal Privoznik mpriv...@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 :)

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

2012-02-29 Thread Michael Roth 
On Wed, Feb 29, 2012 at 12:01:16PM -0300, Luiz Capitulino wrote:
 On Wed, 29 Feb 2012 14:17:52 +0100
 Michal Privoznik mpriv...@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.

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

2012-02-28 Thread Luiz Capitulino 
On Mon, 27 Feb 2012 20:07:28 -0600
Michael Roth mdr...@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.

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

2012-02-28 Thread Michael Roth 
On Tue, Feb 28, 2012 at 11:22:22AM -0300, Luiz Capitulino wrote:
 On Mon, 27 Feb 2012 20:07:28 -0600
 Michael Roth mdr...@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.

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

2012-02-28 Thread Luiz Capitulino 
On Tue, 28 Feb 2012 11:09:38 -0600
Michael Roth mdr...@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 mdr...@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.

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

2012-02-27 Thread Michal Privoznik 
On 23.02.2012 15:20, Luiz Capitulino wrote:
 On Sun, 19 Feb 2012 12:15:43 +0100
 Michal Privoznik mpriv...@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 mpriv...@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 listinterface; 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

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

2012-02-27 Thread 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 mpriv...@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 mpriv...@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 listinterface; 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':

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

2012-02-23 Thread Luiz Capitulino 
On Sun, 19 Feb 2012 12:15:43 +0100
Michal Privoznik mpriv...@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 mpriv...@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  mdr...@linux.vnet.ibm.com
 + *  Michal Privoznik  mpriv...@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));
 +

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

2012-02-22 Thread Michal Privoznik 
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 mpriv...@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?

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

2012-02-19 Thread Michal Privoznik 
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 mpriv...@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  mdr...@linux.vnet.ibm.com
+ *  Michal Privoznik  mpriv...@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);
+