[libvirt] [PATCHv4 0/4] Introduce APIs to extract DHCP leases info
This API returns the leases information stored in the DHCP leases file of dnsmasq for a given virtual network. It contacts the bridge network driver, which parses the leases file. It supports two methods: 1. Return info for all network interfaces connected to a given virtual network 2. Return information for a particular network interface in a given virtual network by providing its MAC Address v4 * Added support for DHCPv6, updated lease file parsing method v3 * Mostly small nits, change in MACRO names, use of virSocketAddrGetIpPrefix to retrieve IP prefix from @dom XML. Refer: https://www.redhat.com/archives/libvir-list/2013-September/msg00832.html v2 * Since DHCPv6 is supposed to be suported in future, virNetworkGetDHCPLeasesForMAC changed, prefix and virIPAddrType added in virNetworkDHCPLeases struct. Refer: https://www.redhat.com/archives/libvir-list/2013-September/msg00732.html v1 * Refer: https://www.redhat.com/archives/libvir-list/2013-September/msg00620.html * The need for these APIs were result of a RFC was proposed on the list. Refer: http://www.redhat.com/archives/libvir-list/2013-July/msg01603.html Nehal J Wani (4): net-dhcp-leases: Implement the remote protocol net-dhcp-leases: Implement the remote protocol net-dhcp-leases: Private implementation inside network driver net-dhcp-leases: Add virsh support daemon/remote.c | 157 ++++++++++++++++++++++++++++ include/libvirt/libvirt.h.in | 36 +++++++ python/generator.py | 3 + src/driver.h | 13 +++ src/libvirt.c | 177 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 3 + src/network/bridge_driver.c | 240 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 169 +++++++++++++++++++++++++++++- src/remote/remote_protocol.x | 59 ++++++++++- src/remote_protocol-structs | 47 +++++++++ src/rpc/gendispatch.pl | 1 + tools/virsh-network.c | 150 +++++++++++++++++++++++++++ tools/virsh.pod | 6 ++ 13 files changed, 1057 insertions(+), 4 deletions(-) -- 1.7.11.7
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree. * virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network. For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL) For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID * virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address. * virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently. There is no support for flags, so user is expected to pass 0 for both the APIs. include/libvirt/libvirt.h.in: * Define virNetworkGetDHCPLeases * Define virNetworkGetDHCPLeasesForMAC * Define virNetworkDHCPLeaseFree python/generator.py: * Skip the auto-generation for virNetworkGetDHCPLeases * Skip the auto-generation for virNetworkGetDHCPLeasesForMAC * Skip the auto-generation for virNetworkDHCPLeaseFree src/driver.h: * Define networkGetDHCPLeases * Define networkGetDHCPLeasesForMAC src/libvirt.c: * Implement virNetworkGetDHCPLeases * Implement virNetworkGetDHCPLeasesForMAC * Implement virNetworkDHCPLeaseFree src/libvirt_public.syms: * Export the new symbols --- include/libvirt/libvirt.h.in | 36 +++++++++ python/generator.py | 3 + src/driver.h | 13 ++++ src/libvirt.c | 177 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 3 + 5 files changed, 232 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 83c219e..557a4c1 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2800,6 +2800,42 @@ int virConnectNumOfDefinedInterfaces (virConnectPtr conn); int virConnectListDefinedInterfaces (virConnectPtr conn, char **const names, int maxnames); + +typedef enum { + VIR_IP_ADDR_TYPE_IPV4, + VIR_IP_ADDR_TYPE_IPV6, + +#ifdef VIR_ENUM_SENTINELS + VIR_IP_ADDR_TYPE_LAST +#endif +} virIPAddrType; + +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id; + char *ipaddr; /* IP address */ + char *hostname; /* Hostname */ + char *clientid; /* Client ID or DUID */ + int type; /* virIPAddrType */ + unsigned int prefix; /* IP address prefix */ +}; + +void virNetworkDHCPLeaseFree(virNetworkDHCPLeasesPtr lease); + +int virNetworkGetDHCPLeases(virNetworkPtr network, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags); + +int virNetworkGetDHCPLeasesForMAC(virNetworkPtr network, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags); + /* * virConnectListAllInterfaces: * diff --git a/python/generator.py b/python/generator.py index 12c14f1..1079d6c 100755 --- a/python/generator.py +++ b/python/generator.py @@ -461,6 +461,8 @@ skip_impl = ( 'virNodeGetCPUMap', 'virDomainMigrate3', 'virDomainMigrateToURI3', + 'virNetworkGetDHCPLeases', + 'virNetworkGetDHCPLeasesForMAC', ) lxc_skip_impl = ( @@ -561,6 +563,7 @@ skip_function = ( "virTypedParamsGetString", "virTypedParamsGetUInt", "virTypedParamsGetULLong", + 'virNetworkDHCPLeaseFree', ) lxc_skip_function = ( diff --git a/src/driver.h b/src/driver.h index 8cd164a..e000b17 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1127,6 +1127,17 @@ typedef int int cookieinlen, unsigned int flags, int cancelled); +typedef int +(*virDrvNetworkGetDHCPLeases)(virNetworkPtr network, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags); + +typedef int +(*virDrvNetworkGetDHCPLeasesForMAC)(virNetworkPtr network, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags); + typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr; @@ -1458,6 +1469,8 @@ struct _virNetworkDriver { virDrvNetworkSetAutostart networkSetAutostart; virDrvNetworkIsActive networkIsActive; virDrvNetworkIsPersistent networkIsPersistent; + virDrvNetworkGetDHCPLeases networkGetDHCPLeases; + virDrvNetworkGetDHCPLeasesForMAC networkGetDHCPLeasesForMAC; }; diff --git a/src/libvirt.c b/src/libvirt.c index 9f579a6..93ea3bf 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -68,6 +68,7 @@ #include "virstring.h" #include "virutil.h" #include "virtypedparam.h" +#include "virmacaddr.h" #ifdef WITH_TEST # include "test/test_driver.h" @@ -22018,3 +22019,179 @@ error: virDispatchError(dom->conn); return -1; } + +/** + * virNetworkGetDHCPLeases: + * @network: Pointer to network object + * @leases: Pointer to a variable to store the array containing details on + * obtained leases, or NULL if the list is not required (just returns + * number of leases). + * @flags: Extra flags, not used yet, so callers should always pass 0 + * + * The API fetches lease info for all network interfaces connected to the given + * virtual network. In case of DHCPv4, the fields returned are expiry time, MAC + * address, IPv4 address, hostname and client ID, out of which, hostname and + * client ID can be NULL. In case of DHCPv6, the fields returned are expiry time, + * IAID, IPv6 address, hostname and client DUID, out of which, only hostname can + * be NULL. + * + * Returns the number of leases found or -1 and sets @leases to NULL in case of + * error. On success, the array stored into @leases is guaranteed to have an + * extra allocated element set to NULL but not included in the return count, + * to make iteration easier. The caller is responsible for calling + * virNetworkDHCPLeaseFree() on each array element, then calling free() on @leases. + * + * Example of usage: + * + * virNetworkDHCPLeasesPtr *leases = NULL; + * virNetworkPtr network = ... obtain a network pointer here ...; + * size_t i; + * int nleases; + * unsigned int flags = 0; + * + * nleases = virNetworkGetDHCPLeases(network, &leases, flags); + * if (nleases < 0) + * error(); + * + * ... do something with returned values, for example: + * + * for (i = 0; i < nleases; i++) { + * virNetworkDHCPLeasesPtr lease = leases[i]; + * + * printf("Time(epoch): %lu, MAC address: %s, " + * "IP address: %s, Hostname: %s, ClientID: %s\n", + * leas->expirytime, lease->mac, lease->ipaddr, + * lease->hostname, lease->clientid); + * + * virNetworkDHCPLeaseFree(leases[i]); + * } + * + * free(leases); + * + */ +int +virNetworkGetDHCPLeases(virNetworkPtr network, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("network=%p, leases=%p, flags=%x", + network, leases, flags); + + virResetLastError(); + + if (leases) + *leases = NULL; + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = network->conn; + + if (conn->networkDriver && conn->networkDriver->networkGetDHCPLeases) { + int ret; + ret = conn->networkDriver->networkGetDHCPLeases(network,leases, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(network->conn); + return -1; +} + +/** + * virNetworkGetDHCPLeasesForMAC: + * @network: Pointer to network object + * @mac: ASCII formatted MAC address of an interface + * @leases: Pointer to a variable to store the array containing details on + * obtained leases, or NULL if the list is not required (just returns + * number of leases). + * @flags: extra flags, not used yet, so callers should always pass 0 + * + * The API fetches leases info of the interface which matches with the + * given @mac. There can be multiple leases for a single @mac because this + * API supports DHCPv6 too. + * + * Returns the number of leases found or -1 and sets @leases to NULL in case of + * error. On success, the array stored into @leases is guaranteed to have an + * extra allocated element set to NULL but not included in the return count, + * to make iteration easier. The caller is responsible for calling + * virNetworkDHCPLeaseFree() on each array element, then calling free() on @leases. + */ +int +virNetworkGetDHCPLeasesForMAC(virNetworkPtr network, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + virConnectPtr conn; + virMacAddr addr; + + VIR_DEBUG("network=%p, mac=%s, leases=%p, flags=%x", + network, mac, leases, flags); + + virResetLastError(); + + if (leases) + *leases = NULL; + + virCheckNonNullArgGoto(mac, error); + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (virMacAddrParse(mac, &addr) < 0) { + virReportInvalidArg(mac, "%s", + _("Given MAC Address doesn't comply " + "with the standard (IEEE 802) format")); + goto error; + } + + conn = network->conn; + + if (conn->networkDriver && + conn->networkDriver->networkGetDHCPLeasesForMAC) { + int ret; + ret = conn->networkDriver->networkGetDHCPLeasesForMAC(network, mac, + leases, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(network->conn); + return -1; +} + +/** + * virNetworkDHCPLeaseFree: + * @lease: pointer to a leases object + * + * Frees all the memory occupied by @lease. + */ +void +virNetworkDHCPLeaseFree(virNetworkDHCPLeasesPtr lease) +{ + if (!lease) + return; + if (lease->type == VIR_IP_ADDR_TYPE_IPV4) + VIR_FREE(lease->id.mac); + VIR_FREE(lease->ipaddr); + VIR_FREE(lease->hostname); + VIR_FREE(lease->clientid); + VIR_FREE(lease); +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index fe9b497..a07be7e 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -636,7 +636,10 @@ LIBVIRT_1.1.1 { LIBVIRT_1.1.3 { global: + virNetworkDHCPLeaseFree; + virNetworkGetDHCPLeases; virConnectGetCPUModelNames; + virNetworkGetDHCPLeasesForMAC; } LIBVIRT_1.1.1; # .... define new API here using predicted next version number .... -- 1.7.11.7
Since we have lost the train for 1.1.3, changes in a/src/libvirt_public.syms attached. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
Rectified attachment. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
On 10/01/2013 04:16 PM, Nehal J Wani wrote:
Rectified attachment.
A patch of a patch is hard to review; you're better off reposting a clean v5. That said, I'll still try to review, so you may want to wait for comments so you can save some churn by fixing those in your v5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
+ char *ipaddr; /* IP address */ + char *hostname; /* Hostname */
Document that this field can be NULL.
+ char *clientid; /* Client ID or DUID */
Likewise.
+ int type; /* virIPAddrType */
If you DO keep the union (which I'm not too sure about), then convention is to put the discriminator first, not separated by other fields.
+ unsigned int prefix; /* IP address prefix */
This field may be nicer if placed next to ipaddr, since it is more closely related to that field. I sense some struggle in getting the interface right - for everyone's sake, let's FIRST get the interface nailed down, rather than churning on implementations of something that then has to be reworked because the interface wasn't correct.
+ +/** + * virNetworkGetDHCPLeases: + * @network: Pointer to network object + * @leases: Pointer to a variable to store the array containing details on + * obtained leases, or NULL if the list is not required (just returns + * number of leases). + * @flags: Extra flags, not used yet, so callers should always pass 0 + * + * The API fetches lease info for all network interfaces connected to the given + * virtual network. In case of DHCPv4, the fields returned are expiry time, MAC + * address, IPv4 address, hostname and client ID, out of which, hostname and + * client ID can be NULL. In case of DHCPv6, the fields returned are expiry time, + * IAID, IPv6 address, hostname and client DUID, out of which, only hostname can + * be NULL. + * + * Returns the number of leases found or -1 and sets @leases to NULL in case of + * error. On success, the array stored into @leases is guaranteed to have an + * extra allocated element set to NULL but not included in the return count, + * to make iteration easier. The caller is responsible for calling + * virNetworkDHCPLeaseFree() on each array element, then calling free() on @leases. + * + * Example of usage: + * + * virNetworkDHCPLeasesPtr *leases = NULL; + * virNetworkPtr network = ... obtain a network pointer here ...; + * size_t i; + * int nleases; + * unsigned int flags = 0; + * + * nleases = virNetworkGetDHCPLeases(network, &leases, flags); + * if (nleases < 0) + * error(); + * + * ... do something with returned values, for example: + * + * for (i = 0; i < nleases; i++) { + * virNetworkDHCPLeasesPtr lease = leases[i]; + * + * printf("Time(epoch): %lu, MAC address: %s, " + * "IP address: %s, Hostname: %s, ClientID: %s\n", + * leas->expirytime, lease->mac, lease->ipaddr, + * lease->hostname, lease->clientid);
This example does not match the struct above (at least lease-mac would have to be converted to lease->id.mac, if you keep the union). Make sure that the example you list in the documentation comments would actually compile, if someone copies it into their code. Might be worth adding: See also virNetworkGetDHCPLeasesForMAC() as a convenience for filtering the list to a single MAC address.
+ + if (conn->networkDriver && conn->networkDriver->networkGetDHCPLeases) { + int ret; + ret = conn->networkDriver->networkGetDHCPLeases(network,leases, flags);
Space after comma.
+/** + * virNetworkGetDHCPLeasesForMAC: + * @network: Pointer to network object + * @mac: ASCII formatted MAC address of an interface + * @leases: Pointer to a variable to store the array containing details on + * obtained leases, or NULL if the list is not required (just returns + * number of leases). + * @flags: extra flags, not used yet, so callers should always pass 0 + * + * The API fetches leases info of the interface which matches with the + * given @mac. There can be multiple leases for a single @mac because this + * API supports DHCPv6 too. + * + * Returns the number of leases found or -1 and sets @leases to NULL in case of + * error. On success, the array stored into @leases is guaranteed to have an + * extra allocated element set to NULL but not included in the return count, + * to make iteration easier. The caller is responsible for calling + * virNetworkDHCPLeaseFree() on each array element, then calling free() on @leases.
Might be worth adding: See virNetworkGetDHCPLeases() for more details on list contents and proper cleanup. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
I sense some struggle in getting the interface right - for everyone's sake, let's FIRST get the interface nailed down, rather than churning on implementations of something that then has to be reworked because the interface wasn't correct.
Agreed. I hope we can have some discussion on whether we should be supporting DHCPv6 or not, and how should IAID be exposed, if we do. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
According to commit 2d5cd1d724084d9975b2514fb31776627acbe997, libvirt supports DHCPv6. The virNetworkGetDHCPLeases API doesn't mention anywhere (in its name) that it is specific to DHCPv4. Hence I thought of adding the support for DHCPv6. Since the format for DHCPv6 leases is a bit different, it might be difficult to add it in the future by modifying the API. To understand the format for DHCPv6 better, one can follow the conversation: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2013q3/007538.html and the RFC: https://tools.ietf.org/html/rfc3315 -- Nehal J Wani
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
So in case DHCPv6 is used by the client, should we report the rest of the lease fields and report MAC as NULL?
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad http://commandlinewani.blogspot.com
On Wed, Oct 02, 2013 at 03:08:00PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
So in case DHCPv6 is used by the client, should we report the rest of the lease fields and report MAC as NULL?
No, we must report the MAC. This data is useless without the MAC address being present. You can't even implement the virNetworkGetDHCPLeasesForMAC API without knowing the MAC for a lease. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 2, 2013 at 3:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Oct 02, 2013 at 03:08:00PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
So in case DHCPv6 is used by the client, should we report the rest of the lease fields and report MAC as NULL?
No, we must report the MAC. This data is useless without the MAC address being present.
You can't even implement the virNetworkGetDHCPLeasesForMAC API without knowing the MAC for a lease.
The issue is, in case of leases containing ipv6 addresses, there is no field for MAC address. Laine suggested extracting MAC address from the cliend DUID. For example: 1380692760 52:54:00:e7:85:1e 192.168.122.116 * * duid 00:01:00:01:19:dd:fb:37:f0:4d:a2:8c:14:51 1380692762 15172894 2001:db8:ca2:2:1::de * 00:01:00:01:19:dd:fb:af:52:54:00:e7:85:1e The last 17 chars of the client DUID represent the MAC address [00:01:00:01:19:dd:fb:af:((52:54:00:e7:85:1e))]. But RFC3315 strictly suggests: """ Clients and servers MUST treat DUIDs as opaque values and MUST only compare DUIDs for equality. Clients and servers MUST NOT in any other way interpret DUIDs. Clients and servers MUST NOT restrict DUIDs to the types defined in this document, as additional DUID types may be defined in the future. """ So, should we ignore leases containing ipv6 addresses? -- Nehal J Wani
On Wed, Oct 02, 2013 at 03:41:56PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 3:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Oct 02, 2013 at 03:08:00PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
So in case DHCPv6 is used by the client, should we report the rest of the lease fields and report MAC as NULL?
No, we must report the MAC. This data is useless without the MAC address being present.
You can't even implement the virNetworkGetDHCPLeasesForMAC API without knowing the MAC for a lease.
The issue is, in case of leases containing ipv6 addresses, there is no field for MAC address. Laine suggested extracting MAC address from the cliend DUID. For example:
1380692760 52:54:00:e7:85:1e 192.168.122.116 * * duid 00:01:00:01:19:dd:fb:37:f0:4d:a2:8c:14:51 1380692762 15172894 2001:db8:ca2:2:1::de * 00:01:00:01:19:dd:fb:af:52:54:00:e7:85:1e
We don't want to design the API around the limitations of one particular DHCP server implementation. If dnsmasq's leases file can't give us the MAC addr, we still want to allow for MAC in the public struct. Have you asked the dnsmasq developers if they're willing to add a field for the MAC addr to the leases file for IPv6 ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 2, 2013 at 4:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Oct 02, 2013 at 03:41:56PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 3:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Oct 02, 2013 at 03:08:00PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote: > Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC > and virNetworkDHCPLeaseFree. > > * virNetworkGetDHCPLeases: returns the dhcp leases information for a given > virtual network. > > For DHCPv4, the information includes: > - Expirytime > - MAC Address > - IPv4 address (with type and prefix) > - Hostname (can be NULL) > - Client ID (can be NULL) > > For DHCPv6, the information includes > - Expirytime > - IAID > - IPv6 address (with type and prefix) > - Hostname (can be NULL) > - Client DUID > > * virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a > given virtual network and specified MAC Address. > > * virNetworkDHCPLeaseFree: allows the upper layer application to free the > network interface object conveniently. > > There is no support for flags, so user is expected to pass 0 for > both the APIs.
> +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; > +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; > +struct _virNetworkDHCPLeases { > + long long expirytime; /* Seconds since epoch */ > + union { > + char *mac; /* MAC address */ > + unsigned long iaid; /* Identity association identifier (IAID) */ > + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
So in case DHCPv6 is used by the client, should we report the rest of the lease fields and report MAC as NULL?
No, we must report the MAC. This data is useless without the MAC address being present.
You can't even implement the virNetworkGetDHCPLeasesForMAC API without knowing the MAC for a lease.
The issue is, in case of leases containing ipv6 addresses, there is no field for MAC address. Laine suggested extracting MAC address from the cliend DUID. For example:
1380692760 52:54:00:e7:85:1e 192.168.122.116 * * duid 00:01:00:01:19:dd:fb:37:f0:4d:a2:8c:14:51 1380692762 15172894 2001:db8:ca2:2:1::de * 00:01:00:01:19:dd:fb:af:52:54:00:e7:85:1e
We don't want to design the API around the limitations of one particular DHCP server implementation. If dnsmasq's leases file can't give us the MAC addr, we still want to allow for MAC in the public struct. Have you asked the dnsmasq developers if they're willing to add a field for the MAC addr to the leases file for IPv6 ?
I think one of them answered this question in the thread: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2013q3/007538.html One has to read all the follow-ups :) For a tl;dr moment: """ However, if you're interested in the MAC addresses of clients, the very latest dnsmasq code can determine that in most cases. The MAC address is not stored in the leases file, but it can be used to key configurations to particular MAC addresses, and it's made available to the DHCP lease external script, so an external application can use it. """ """ I realise that adding to MAC to the leasefile for DHCPv6 clients would make this possible, but I'm reluctant to change the leasefile format. """ -- Nehal J Wani
On Wed, Oct 02, 2013 at 04:30:16PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 4:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Oct 02, 2013 at 03:41:56PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 3:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Oct 02, 2013 at 03:08:00PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote: > On 09/26/2013 02:08 AM, Nehal J Wani wrote: > > Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC > > and virNetworkDHCPLeaseFree. > > > > * virNetworkGetDHCPLeases: returns the dhcp leases information for a given > > virtual network. > > > > For DHCPv4, the information includes: > > - Expirytime > > - MAC Address > > - IPv4 address (with type and prefix) > > - Hostname (can be NULL) > > - Client ID (can be NULL) > > > > For DHCPv6, the information includes > > - Expirytime > > - IAID > > - IPv6 address (with type and prefix) > > - Hostname (can be NULL) > > - Client DUID > > > > * virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a > > given virtual network and specified MAC Address. > > > > * virNetworkDHCPLeaseFree: allows the upper layer application to free the > > network interface object conveniently. > > > > There is no support for flags, so user is expected to pass 0 for > > both the APIs. > > > > +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; > > +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; > > +struct _virNetworkDHCPLeases { > > + long long expirytime; /* Seconds since epoch */ > > + union { > > + char *mac; /* MAC address */ > > + unsigned long iaid; /* Identity association identifier (IAID) */ > > + } id; > > I'm not sure I like iaid - the whole point of this interface was to > return IP addresses associated with a MAC. Either the iaid is important > and deserves a separate field, or all we care about is the MAC address. > Not to mention that you didn't document which leg of the id union is > valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
So in case DHCPv6 is used by the client, should we report the rest of the lease fields and report MAC as NULL?
No, we must report the MAC. This data is useless without the MAC address being present.
You can't even implement the virNetworkGetDHCPLeasesForMAC API without knowing the MAC for a lease.
The issue is, in case of leases containing ipv6 addresses, there is no field for MAC address. Laine suggested extracting MAC address from the cliend DUID. For example:
1380692760 52:54:00:e7:85:1e 192.168.122.116 * * duid 00:01:00:01:19:dd:fb:37:f0:4d:a2:8c:14:51 1380692762 15172894 2001:db8:ca2:2:1::de * 00:01:00:01:19:dd:fb:af:52:54:00:e7:85:1e
We don't want to design the API around the limitations of one particular DHCP server implementation. If dnsmasq's leases file can't give us the MAC addr, we still want to allow for MAC in the public struct. Have you asked the dnsmasq developers if they're willing to add a field for the MAC addr to the leases file for IPv6 ?
I think one of them answered this question in the thread: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2013q3/007538.html One has to read all the follow-ups :)
For a tl;dr moment:
""" However, if you're interested in the MAC addresses of clients, the very latest dnsmasq code can determine that in most cases. The MAC address is not stored in the leases file, but it can be used to key configurations to particular MAC addresses, and it's made available to the DHCP lease external script, so an external application can use it. """
So we can get the MAC addr if we use the '--dhcp-script' parameter to make dnsmasq invoke a helper program we create, to save the lease details we need. Incidentally, I see our XML format is actually wrong, because it says the 'mac' attribute is forbidden for <host> elements using IPv6. dnsmasq actually allows you to specify either mac addr or IAID identifiers for IPv6 dhcp host entries. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
So we can get the MAC addr if we use the '--dhcp-script' parameter to make dnsmasq invoke a helper program we create, to save the lease details we need.
Incidentally, I see our XML format is actually wrong, because it says the 'mac' attribute is forbidden for <host> elements using IPv6. dnsmasq actually allows you to specify either mac addr or IAID identifiers for IPv6 dhcp host entries.
I gave a try to the --dhcp-script option of dnsmasq. Following are the findings: Script used" (a little modified version of http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=blob_plain;f=contrib/mactab...): #!/bin/bash STATUS_FILE="/var/lib/libvirt/dnsmasq/dnsmasq-ip-mac.status" # Script for dnsmasq lease-change hook. # Maintains the above file with a IP address/MAC address pairs, # one lease per line. Works with IPv4 and IPv6 leases, file is # atomically updated, so no races for users of the data. action="$1" mac="$2" # IPv4 ip="$3" expirytime="$DNSMASQ_LEASE_EXPIRES" hostname="$DNSMASQ_SUPPLIED_HOSTNAME" clientid="$DNSMASQ_CLIENT_ID" # ensure it always exists. if [ ! -f "$STATUS_FILE" ]; then touch "$STATUS_FILE" fi if [ -n "$DNSMASQ_IAID" ]; then mac="$DNSMASQ_MAC" # IPv6 clientid="$2" fi # worry about an add or old action when the MAC address is not known: # leave any old one in place in that case. if [ "$action" = "add" -o "$action" = "old" -o "$action" = "del" ]; then if [ -n "$mac" -o "$action" = "del" ]; then sed "/^${ip//./\.} / d" "$STATUS_FILE" > "$STATUS_FILE".new if [ "$action" = "add" -o "$action" = "old" ]; then echo "$expirytime $mac $ip $hostname $clientid" >> "$STATUS_FILE".new fi mv "$STATUS_FILE".new "$STATUS_FILE" # atomic update. fi fi Changes made to libvirt code: diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8787bdb..7f9a74f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1058,6 +1058,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + virCommandAddArgFormat(cmd, "--dhcp-script=%s", "/var/lib/libvirt/dnsmasq/macscript.sh"); *cmdout = cmd; ret = 0; cleanup: In dnsmasq version 2.65 (latest on f18 repos), useful variables that were set: In the case of ipv4: $1=add $2=52:54:00:95:41:5d $3=192.168.100.128 DNSMASQ_INTERFACE=virbr0 DNSMASQ_TAGS=virbr0 DNSMASQ_TIME_REMAINING=3600 DNSMASQ_LEASE_EXPIRES=1380745674 In the case of ipv6: $1=add $2=00:01:00:01:19:df:2e:19:52:54:00:24:13:15 $3=2001:db8:ca2:2:1::45 DNSMASQ_INTERFACE=virbr3 DNSMASQ_TAGS=dhcpv6 virbr3 DNSMASQ_SERVER_DUID=00:01:00:01:19:df:29:7e:f0:4d:a2:8c:14:51 DNSMASQ_IAID=2364181 DNSMASQ_TIME_REMAINING=3600 DNSMASQ_LEASE_EXPIRES=1380745131 In the latest dnsmasq version 2.67rc2-3-g889d8a1 (built after cloning from git://thekelleys.org.uk/dnsmasq.git), useful variables that were set: In the case of ipv4: add 52:54:00:1a:a1:55 192.168.100.204 DNSMASQ_INTERFACE=virbr0 DNSMASQ_LEASE_EXPIRES=1380749702 DNSMASQ_TAGS=virbr0 DNSMASQ_TIME_REMAINING=3600 In the case of ipv6: add 00:01:00:01:19:df:3a:8e:52:54:00:7d:49:25 2001:db8:ca2:2:1::f5 DNSMASQ_IAID=8210725 DNSMASQ_INTERFACE=virbr3 DNSMASQ_LEASE_EXPIRES=1380748320 DNSMASQ_MAC=52:54:00:7d:49:25 DNSMASQ_SERVER_DUID= DNSMASQ_TAGS=dhcpv6 virbr3 DNSMASQ_TIME_REMAINING=3600 So, in case of latest dnsmasq code, output in dnsmasq-ip-mac.status: 1380747917 52:54:00:82:5e:09 2001:db8:ca2:2:1::79 1380747943 52:54:00:61:bd:d8 2001:db8:ca2:2:1::88 1380748110 52:54:00:15:1e:05 192.168.100.180 1380748320 52:54:00:7d:49:25 2001:db8:ca2:2:1::f5 00:01:00:01:19:df:3a:8e:52:54:00:7d:49:25 1380749702 52:54:00:1a:a1:55 192.168.100.204 1380749877 52:54:00:73:0a:27 192.168.100.190 1380749879 52:54:00:b7:87:3e 2001:db8:ca2:2:1::3e 00:01:00:01:19:df:40:a6:52:54:00:b7:87:3e 1380749880 52:54:00:bc:55:df 2001:db8:ca2:2:1::8f 00:01:00:01:19:df:40:a6:52:54:00:bc:55:df 1380749880 52:54:00:b7:87:3e 2001:db8:ca2:2:1::3e 00:01:00:01:19:df:40:a6:52:54:00:b7:87:3e So, I think it is OK to parse the custom generated file: dnsmasq-ip-mac.status (after deciding its exact format). But the only issue right now is that the variable DNSMASQ_MAC for DHCPv6 is set only in the case of latest dnsmasq code (I don't think it is even available in the tarballs yet) -- Nehal J Wani
The dnsmasq guys have confirmed that the code exposing the environment variable DNSMASQ_MAC in case ipv6 will be out in the coming week as an official release. Meanwhile, I had a chat with mrjester on #ipv6 regarding fields which are supposed to be useful to users in case DHCPv6 is used. I've attached the chat history. Now, coming to the API, I do think that supporting --dhcp-script will be a good option. So that we don't land up with half-implementations. So I am hoping that we are in a position to finalize the virNetworkDHCPLeases struct now. In case of DHCPv6, we should be exposing expirytime, MAC Address, IP Address, hostname, client DUID, IAID and the interface name to which the lease is provided (which can be obtained from the env variable DNSMASQ_INTERFACE) I personally think the easiest way would be to have all possible values in the virNetworkDHCPLeases struct and let the user decide which one he wants. The ones which are not available, will be set to NULL. -- Nehal J Wani
On Fri, Oct 4, 2013 at 9:27 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
The dnsmasq guys have confirmed that the code exposing the environment variable DNSMASQ_MAC in case ipv6 will be out in the coming week as an official release. Meanwhile, I had a chat with mrjester on #ipv6 regarding fields which are supposed to be useful to users in case DHCPv6 is used. I've attached the chat history.
Now, coming to the API, I do think that supporting --dhcp-script will be a good option. So that we don't land up with half-implementations. So I am hoping that we are in a position to finalize the virNetworkDHCPLeases struct now.
In case of DHCPv6, we should be exposing expirytime, MAC Address, IP Address, hostname, client DUID, IAID and the interface name to which the lease is provided (which can be obtained from the env variable DNSMASQ_INTERFACE)
I personally think the easiest way would be to have all possible values in the virNetworkDHCPLeases struct and let the user decide which one he wants. The ones which are not available, will be set to NULL.
-- Nehal J Wani
ping If all are OK with the inclusion of --dhcp-script, I would like to finalize the design of virNetworkDHCPLeases struct. -- Nehal J Wani
On 10/08/2013 05:31 AM, Nehal J Wani wrote:
I personally think the easiest way would be to have all possible values in the virNetworkDHCPLeases struct and let the user decide which one he wants. The ones which are not available, will be set to NULL.
-- Nehal J Wani
ping
If all are OK with the inclusion of --dhcp-script, I would like to finalize the design of virNetworkDHCPLeases struct.
Yes, the idea of --dhcp-script is reasonable, and yes, finalizing the struct design is the right step to take. The best action now is to start a new thread with the proposed struct design, as it is easier to discuss design that isn't buried inside a larger thread. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/02/2013 06:11 AM, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 3:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Oct 02, 2013 at 03:08:00PM +0530, Nehal J Wani wrote:
On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id; I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator. Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
So in case DHCPv6 is used by the client, should we report the rest of the lease fields and report MAC as NULL? No, we must report the MAC. This data is useless without the MAC address being present.
You can't even implement the virNetworkGetDHCPLeasesForMAC API without knowing the MAC for a lease. The issue is, in case of leases containing ipv6 addresses, there is no field for MAC address. Laine suggested extracting MAC address from the cliend DUID. For example:
1380692760 52:54:00:e7:85:1e 192.168.122.116 * * duid 00:01:00:01:19:dd:fb:37:f0:4d:a2:8c:14:51 1380692762 15172894 2001:db8:ca2:2:1::de * 00:01:00:01:19:dd:fb:af:52:54:00:e7:85:1e
The last 17 chars of the client DUID represent the MAC address [00:01:00:01:19:dd:fb:af:((52:54:00:e7:85:1e))]. But RFC3315 strictly suggests: """ Clients and servers MUST treat DUIDs as opaque values and MUST only compare DUIDs for equality. Clients and servers MUST NOT in any other way interpret DUIDs. Clients and servers MUST NOT restrict DUIDs to the types defined in this document, as additional DUID types may be defined in the future.
You aren't a client or a server (i.e. you aren't participating in the protocol). You are a third party who is already invading the internal state data store of one particular implementation of DHCP server - dnsmasq (and only dnsmasq) - to try and mine some useful information. It is a happy coincidence that the MAC address is contained in the DUID in dnsmasq, but it *is* there, and doesn't appear to be available anywhere else (without snooping packets on the bridge for a matching IP address). So your choices are use that, snoop the packets, or go home. (You should not, however, attempt to write a general purpose function intended to derive the MAC address from the DUID of a lease for *any* DHCP implementation - *that* is what the RFC says you must not do) BTW, I agree with Dan that all we care about are IP address and MAC address - IAID and DUID are just artifacts of the method used to assign an IP address, and won't mean anything to anybody except the DHCP server and client software.
On Wed, Oct 02, 2013 at 09:13:59AM +0100, Daniel P. Berrange wrote:
On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote:
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
* virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network.
For DHCPv4, the information includes: - Expirytime - MAC Address - IPv4 address (with type and prefix) - Hostname (can be NULL) - Client ID (can be NULL)
For DHCPv6, the information includes - Expirytime - IAID - IPv6 address (with type and prefix) - Hostname (can be NULL) - Client DUID
* virNetworkGetDHCPLeasesForMAC: returns the dhcp leases information for a given virtual network and specified MAC Address.
* virNetworkDHCPLeaseFree: allows the upper layer application to free the network interface object conveniently.
There is no support for flags, so user is expected to pass 0 for both the APIs.
+typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */ + } id;
I'm not sure I like iaid - the whole point of this interface was to return IP addresses associated with a MAC. Either the iaid is important and deserves a separate field, or all we care about is the MAC address. Not to mention that you didn't document which leg of the id union is valid based on the type discriminator.
Agreed, we want the MAC address to be unconditionally available here. IMHO the IAID is not something we care about exposing. That is a impl detail of the DHCP comms protocol that is not useful to people outside.
Actually, I see that we already expose the IAID concept to the user in the XML for networks <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" > <dhcp> <host name="paul" ip="2001:db8:ca2:2:3::1" /> <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66" ip="2001:db8:ca2:2:3::2" /> <host id="0:3:0:1:0:16:3e:11:22:33" name="ralph" ip="2001:db8:ca2:2:3::3" /> <host id="0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63" name="badbob" ip="2001:db8:ca2:2:3::4" /> </dhcp> </ip> The 'id' value is mapping to the IAID. So it is sensible to expose this field in the virNetworkDHCPLease struct, but as a separate field, not as a union. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/02/2013 04:46 AM, Daniel P. Berrange wrote:
Actually, I see that we already expose the IAID concept to the user in the XML for networks
<ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" > <dhcp> <host name="paul" ip="2001:db8:ca2:2:3::1" /> <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66" ip="2001:db8:ca2:2:3::2" />
The 'id' value is mapping to the IAID. So it is sensible to expose this field in the virNetworkDHCPLease struct, but as a separate field, not as a union.
That, and IAID is NOT an 'unsigned long long int', but an arbitrary string, and you should not be trying to parse it into a single integer. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
+ +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime; /* Seconds since epoch */ + union { + char *mac; /* MAC address */ + unsigned long iaid; /* Identity association identifier (IAID) */
'unsigned long' is wrong for any new API. It is platform dependent, and makes us have to worry about a 32-bit client talking to a 64-bit host, and reporting OVERFLOW errors if the host sends back an answer larger than 32 bits. If iaid is truly numeric (which I doubt), then use 'unsigned long long'. But more likely, iaid should be a char* and treated as an opaque string, rather than trying to treat it as always being parseable into an integer. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Implement RPC calls for virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC daemon/remote.c * Define remoteSerializeNetworkDHCPLeases, remoteDispatchNetworkGetDHCPLeases * Define remoteDispatchNetworkGetDHCPLeasesForMAC * Define helper function make_dhcp_lease src/remote/remote_driver.c * Define remoteNetworkGetDHCPLeases * Define remoteNetworkGetDHCPLeasesForMAC * Define helper function make_dhcp_lease src/remote/remote_protocol.x * New RPC procedure: REMOTE_PROC_NETWORK_GET_DHCP_LEASES * Define structs remote_network_dhcp_leases, remote_network_get_dhcp_leases_args, remote_network_get_dhcp_leases_ret * New RPC procedure: REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC * Define structs remote_network_dhcp_leases_for_mac, remote_network_get_dhcp_leases_for_mac_args, remote_network_get_dhcp_leases_for_mac_ret src/remote_protocol-structs * New structs added src/rpc/gendispatch.pl * Add exception (s/Dhcp/DHCP) for auto-generating names of the remote functions in daemon/remote_dispatch.h --- daemon/remote.c | 157 ++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 169 ++++++++++++++++++++++++++++++++++++++++++- src/remote/remote_protocol.x | 59 ++++++++++++++- src/remote_protocol-structs | 47 ++++++++++++ src/rpc/gendispatch.pl | 1 + 5 files changed, 429 insertions(+), 4 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 9497cc1..770f62a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -88,6 +88,7 @@ static void make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNod static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src); static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); +static void make_dhcp_lease(remote_network_dhcp_lease *lease_dst, virNetworkDHCPLeasesPtr lease_src); static virTypedParameterPtr remoteDeserializeTypedParameters(remote_typed_param *args_params_val, @@ -5209,6 +5210,136 @@ cleanup: } +static int +remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_network_get_dhcp_leases_args *args, + remote_network_get_dhcp_leases_ret *ret) +{ + int rv = -1; + size_t i; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virNetworkDHCPLeasesPtr *leases = NULL; + virNetworkPtr net = NULL; + int nleases = 0; + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(net = get_nonnull_network(priv->conn, args->net))) + goto cleanup; + + if ((nleases = virNetworkGetDHCPLeases(net, + args->need_results ? &leases : NULL, + args->flags)) < 0) + goto cleanup; + + if (nleases > REMOTE_NETWORK_DHCP_LEASES_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"), + nleases, REMOTE_NETWORK_DHCP_LEASES_MAX); + return -1; + } + + if (leases && nleases) { + if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + goto cleanup; + + ret->leases.leases_len = nleases; + for (i = 0; i < nleases; i++) + make_dhcp_lease(ret->leases.leases_val + i, leases[i]); + } else { + ret->leases.leases_len = 0; + ret->leases.leases_val = NULL; + } + + ret->ret = nleases; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + for (i = 0; i < nleases; i++) + virNetworkDHCPLeaseFree(leases[i]); + VIR_FREE(leases); + } + virNetworkFree(net); + return rv; +} + + +static int +remoteDispatchNetworkGetDHCPLeasesForMAC(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_network_get_dhcp_leases_for_mac_args *args, + remote_network_get_dhcp_leases_for_mac_ret *ret) +{ + int rv = -1; + size_t i; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virNetworkDHCPLeasesPtr *leases = NULL; + virNetworkPtr net = NULL; + int nleases = 0; + const char *mac = NULL; + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(net = get_nonnull_network(priv->conn, args->net))) + goto cleanup; + + mac = args->mac; + + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac, + args->need_results ? &leases : NULL, + args->flags)) < 0) + goto cleanup; + + if (nleases > REMOTE_NETWORK_DHCP_LEASES_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"), + nleases, REMOTE_NETWORK_DHCP_LEASES_MAX); + return -1; + } + + if (leases && nleases) { + if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + goto cleanup; + + ret->leases.leases_len = nleases; + for (i = 0; i < nleases; i++) + make_dhcp_lease(ret->leases.leases_val + i, leases[i]); + } else { + ret->leases.leases_len = 0; + ret->leases.leases_val = NULL; + } + + ret->ret = nleases; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + for (i = 0; i < nleases; i++) + virNetworkDHCPLeaseFree(leases[i]); + VIR_FREE(leases); + } + virNetworkFree(net); + return rv; +} + /*----- Helpers. -----*/ @@ -5343,6 +5474,32 @@ make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDo make_nonnull_domain(&snapshot_dst->dom, snapshot_src->domain); } + +/* Copy contents of virNetworkDHCPLeasesPtr to remote_network_dhcp_lease */ +static void +make_dhcp_lease(remote_network_dhcp_lease *lease_dst, virNetworkDHCPLeasesPtr lease_src) +{ + char **hostname_tmp = NULL; + char **clientid_tmp = NULL; + ignore_value(VIR_ALLOC_QUIET(hostname_tmp)); + ignore_value(VIR_ALLOC_QUIET(clientid_tmp)); + + lease_dst->expirytime = lease_src->expirytime; + lease_dst->type = lease_src->type; + lease_dst->prefix = lease_src->prefix; + if (lease_src->type == VIR_IP_ADDR_TYPE_IPV4) + ignore_value(VIR_STRDUP_QUIET(lease_dst->id.remote_interface_id_u.mac, + lease_src->id.mac)); + else + lease_dst->id.remote_interface_id_u.iaid = lease_src->id.iaid; + ignore_value(VIR_STRDUP_QUIET(lease_dst->ipaddr, lease_src->ipaddr)); + ignore_value(VIR_STRDUP_QUIET(*hostname_tmp, lease_src->hostname)); + ignore_value(VIR_STRDUP_QUIET(*clientid_tmp, lease_src->clientid)); + lease_dst->hostname = *hostname_tmp ? hostname_tmp : NULL; + lease_dst->clientid = *clientid_tmp ? clientid_tmp : NULL; +} + + static int remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors, int nerrors, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 96ccb99..95910b6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -14,7 +14,7 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. * - * You should have received a copy of the GNU Lesser General Public + * You should have received a make of the GNU Lesser General Public * License along with this library. If not, see * <http://www.gnu.org/licenses/>. * @@ -150,6 +150,7 @@ static void make_nonnull_storage_vol(remote_nonnull_storage_vol *vol_dst, virSto static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src); static void make_nonnull_nwfilter(remote_nonnull_nwfilter *nwfilter_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); +static void make_dhcp_lease(virNetworkDHCPLeasesPtr lease_src, remote_network_dhcp_lease *lease_dst); static void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event); /*----------------------------------------------------------------------*/ @@ -1058,7 +1059,7 @@ doRemoteClose(virConnectPtr conn, struct private_data *priv) virObjectUnref(priv->qemuProgram); priv->remoteProgram = priv->qemuProgram = priv->lxcProgram = NULL; - /* Free hostname copy */ + /* Free hostname make */ VIR_FREE(priv->hostname); /* See comment for remoteType. */ @@ -3756,7 +3757,7 @@ static sasl_callback_t *remoteAuthMakeCallbacks(int *credtype, int ncredtype) * * Builds up an array of libvirt credential structs, populating * with data from the SASL interaction struct. These two structs - * are basically a 1-to-1 copy of each other. + * are basically a 1-to-1 make of each other. */ static int remoteAuthMakeCredentials(sasl_interact_t *interact, virConnectCredentialPtr *cred, @@ -6661,6 +6662,143 @@ done: return rv; } + +static int +remoteNetworkGetDHCPLeases(virNetworkPtr net, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + size_t i; + struct private_data *priv = net->conn->networkPrivateData; + remote_network_get_dhcp_leases_args args; + remote_network_get_dhcp_leases_ret ret; + + virNetworkDHCPLeasesPtr *leases_ret = NULL; + remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.flags = flags; + args.need_results = !!leases; + + memset(&ret, 0, sizeof(ret)); + + if (call(net->conn, priv, 0, REMOTE_PROC_NETWORK_GET_DHCP_LEASES, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_args, (char *)&args, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_ret, (char *)&ret) == -1) { + goto done; + } + + if (ret.leases.leases_len > REMOTE_NETWORK_DHCP_LEASES_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"), + ret.leases.leases_len, REMOTE_NETWORK_DHCP_LEASES_MAX); + goto cleanup; + } + + if (leases) { + if (ret.leases.leases_len && + VIR_ALLOC_N(leases_ret, ret.leases.leases_len + 1) < 0) { + goto cleanup; + } + + for (i = 0; i < ret.leases.leases_len; i++) { + if (VIR_ALLOC(leases_ret[i]) < 0) + goto cleanup; + + make_dhcp_lease(leases_ret[i], &ret.leases.leases_val[i]); + } + + *leases = leases_ret; + leases_ret = NULL; + } + + rv = ret.ret; + +cleanup: + if (leases_ret) { + for (i = 0; i < ret.leases.leases_len; i++) + virNetworkDHCPLeaseFree(leases_ret[i]); + VIR_FREE(leases_ret); + } + xdr_free((xdrproc_t)xdr_remote_network_get_dhcp_leases_ret, + (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + + +static int +remoteNetworkGetDHCPLeasesForMAC(virNetworkPtr net, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + size_t i; + struct private_data *priv = net->conn->networkPrivateData; + remote_network_get_dhcp_leases_for_mac_args args; + remote_network_get_dhcp_leases_for_mac_ret ret; + + virNetworkDHCPLeasesPtr *leases_ret = NULL; + remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.mac = (char *) mac; + args.flags = flags; + args.need_results = !!leases; + + memset(&ret, 0, sizeof(ret)); + + if (call(net->conn, priv, 0, REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_for_mac_args, (char *)&args, + (xdrproc_t)xdr_remote_network_get_dhcp_leases_for_mac_ret, (char *)&ret) == -1) { + goto done; + } + + if (ret.leases.leases_len > REMOTE_NETWORK_DHCP_LEASES_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"), + ret.leases.leases_len, REMOTE_NETWORK_DHCP_LEASES_MAX); + goto cleanup; + } + + if (leases) { + if (ret.leases.leases_len && + VIR_ALLOC_N(leases_ret, ret.leases.leases_len + 1) < 0) { + goto cleanup; + } + + for (i = 0; i < ret.leases.leases_len; i++) { + if (VIR_ALLOC(leases_ret[i]) < 0) + goto cleanup; + + make_dhcp_lease(leases_ret[i], &ret.leases.leases_val[i]); + } + + *leases = leases_ret; + leases_ret = NULL; + } + + rv = ret.ret; + +cleanup: + if (leases_ret) { + for (i = 0; i < ret.leases.leases_len; i++) + virNetworkDHCPLeaseFree(leases_ret[i]); + VIR_FREE(leases_ret); + } + xdr_free((xdrproc_t)xdr_remote_network_get_dhcp_leases_for_mac_ret, + (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + + static void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) { @@ -6793,6 +6931,29 @@ make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDo make_nonnull_domain(&snapshot_dst->dom, snapshot_src->domain); } + +/* Copy contents of remote_network_dhcp_lease to virNetworkDHCPLeasesPtr */ +static void +make_dhcp_lease(virNetworkDHCPLeasesPtr lease_dst, remote_network_dhcp_lease *lease_src) +{ + lease_dst->expirytime = lease_src->expirytime; + lease_dst->type = lease_src->type; + lease_dst->prefix = lease_src->prefix; + if (lease_src->type == VIR_IP_ADDR_TYPE_IPV4) + ignore_value(VIR_STRDUP_QUIET(lease_dst->id.mac, + lease_src->id.remote_interface_id_u.mac)); + else + lease_dst->id.iaid = lease_src->id.remote_interface_id_u.iaid; + ignore_value(VIR_STRDUP_QUIET(lease_dst->ipaddr, lease_src->ipaddr)); + if (lease_src->hostname) + ignore_value(VIR_STRDUP_QUIET(lease_dst->hostname, *lease_src->hostname)); + else + lease_dst->hostname = NULL; + if (lease_src->clientid) + ignore_value(VIR_STRDUP_QUIET(lease_dst->clientid, *lease_src->clientid)); + else + lease_dst->clientid = NULL; +} /*----------------------------------------------------------------------*/ unsigned long remoteVersion(void) @@ -7021,6 +7182,8 @@ static virNetworkDriver network_driver = { .networkSetAutostart = remoteNetworkSetAutostart, /* 0.3.0 */ .networkIsActive = remoteNetworkIsActive, /* 0.7.3 */ .networkIsPersistent = remoteNetworkIsPersistent, /* 0.7.3 */ + .networkGetDHCPLeases = remoteNetworkGetDHCPLeases, /* 1.1.3 */ + .networkGetDHCPLeasesForMAC = remoteNetworkGetDHCPLeasesForMAC, /* 1.1.3 */ }; static virInterfaceDriver interface_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 8d17bad..c630951 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -235,6 +235,11 @@ const REMOTE_DOMAIN_JOB_STATS_MAX = 16; /* Upper limit on number of CPU models */ const REMOTE_CONNECT_CPU_MODELS_MAX = 8192; +/* + * Upper limit on the maximum number of leases in one lease file + */ +const REMOTE_NETWORK_DHCP_LEASES_MAX = 32768; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -2849,6 +2854,46 @@ struct remote_connect_get_cpu_model_names_ret { int ret; }; +union remote_interface_id switch (int type) { +case VIR_IP_ADDR_TYPE_IPV4: + remote_nonnull_string mac; +case VIR_IP_ADDR_TYPE_IPV6: + unsigned hyper iaid; +}; + +struct remote_network_dhcp_lease { + hyper expirytime; + remote_interface_id id; + remote_nonnull_string ipaddr; + remote_string hostname; + remote_string clientid; + int type; + unsigned int prefix; +}; + +struct remote_network_get_dhcp_leases_args { + remote_nonnull_network net; + int need_results; + unsigned int flags; +}; + +struct remote_network_get_dhcp_leases_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>; + unsigned int ret; +}; + +struct remote_network_get_dhcp_leases_for_mac_args { + remote_nonnull_network net; + remote_nonnull_string mac; + int need_results; + unsigned int flags; +}; + +struct remote_network_get_dhcp_leases_for_mac_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>; + unsigned int ret; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -5018,5 +5063,17 @@ enum remote_procedure { * @generate: none * @acl: connect:read */ - REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES = 312 + REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES = 312, + + /** + * @generate: none + * @acl: network:read + */ + REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 313, + + /** + * @generate: none + * @acl: network:read + */ + REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC = 314 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 98d2d5b..b6779d5 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -8,6 +8,10 @@ enum { VIR_TYPED_PARAM_BOOLEAN = 6, VIR_TYPED_PARAM_STRING = 7, }; +enum { + VIR_IP_ADDR_TYPE_IPV4 = 0, + VIR_IP_ADDR_TYPE_IPV6 = 1, +}; struct remote_nonnull_domain { remote_nonnull_string name; remote_uuid uuid; @@ -2328,6 +2332,47 @@ struct remote_connect_get_cpu_model_names_ret { } models; int ret; }; +struct remote_dhcp_interface_id { + int type; + union { + remote_nonnull_string mac; + uint64_t iaid; + } remote_interface_id_u; +}; +struct remote_network_dhcp_lease { + int64_t expirytime; + remote_interface_id id; + remote_nonnull_string ipaddr; + remote_string hostname; + remote_string clientid; + int type; + u_int prefix; +}; +struct remote_network_get_dhcp_leases_args { + remote_nonnull_network net; + int need_results; + u_int flags; +}; +struct remote_network_get_dhcp_leases_ret { + struct { + u_int leases_len; + remote_network_dhcp_lease * leases_val; + } leases; + u_int ret; +}; +struct remote_network_get_dhcp_leases_for_mac_args { + remote_nonnull_network net; + remote_nonnull_string mac; + int need_results; + u_int flags; +}; +struct remote_network_get_dhcp_leases_for_mac_ret { + struct { + u_int leases_len; + remote_network_dhcp_lease * leases_val; + } leases; + u_int ret; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2641,4 +2686,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_CREATE_WITH_FILES = 310, REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES = 312, + REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 313, + REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC = 314, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index ceb1ad8..5a503cf 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -66,6 +66,7 @@ sub fixup_name { $name =~ s/Fstrim$/FSTrim/; $name =~ s/Scsi/SCSI/; $name =~ s/Wwn$/WWN/; + $name =~ s/Dhcp$/DHCP/; return $name; } -- 1.7.11.7
In accordance with latest changes in rules for making building src/remote_protocol-structs, diff attached. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
Rectified attachment. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
On 09/26/2013 02:08 AM, Nehal J Wani wrote:
Implement RPC calls for virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC
--- daemon/remote.c | 157 ++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 169 ++++++++++++++++++++++++++++++++++++++++++- src/remote/remote_protocol.x | 59 ++++++++++++++- src/remote_protocol-structs | 47 ++++++++++++ src/rpc/gendispatch.pl | 1 + 5 files changed, 429 insertions(+), 4 deletions(-)
Reviewing in a different order (pro-tip: you can create a file that lists: *.x *.h * then run 'git diff -O/path/to/file' or 'git send-email -O/path/to/file' and git will automatically arrange the diff in the order that makes more logical sense to reviewers)
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 8d17bad..c630951 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x
@@ -2849,6 +2854,46 @@ struct remote_connect_get_cpu_model_names_ret { int ret; };
+union remote_interface_id switch (int type) { +case VIR_IP_ADDR_TYPE_IPV4: + remote_nonnull_string mac; +case VIR_IP_ADDR_TYPE_IPV6: + unsigned hyper iaid; +}; + +struct remote_network_dhcp_lease { + hyper expirytime; + remote_interface_id id; + remote_nonnull_string ipaddr; + remote_string hostname; + remote_string clientid; + int type;
Similar to 1/4, I'd list 'type' before 'id', rather than after. Once remote_network_dhcp_lease id defined correctly, the rest of this file looks okay.
diff --git a/daemon/remote.c b/daemon/remote.c index 9497cc1..770f62a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -88,6 +88,7 @@ static void make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNod static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src); static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); +static void make_dhcp_lease(remote_network_dhcp_lease *lease_dst, virNetworkDHCPLeasesPtr lease_src);
Can we hoist that function into topological order, instead of having to use a forward declaration? Furthermore, this function really should return an int, so that we can detect OOM failure (yes, the existing make_nonnull_* functions are buggy in that they mishandle OOM, but that's pre-existing and you don't have to worry about it).
+static int +remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_network_get_dhcp_leases_args *args, + remote_network_get_dhcp_leases_ret *ret) +{
+ + if (leases && nleases) { + if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + goto cleanup; + + ret->leases.leases_len = nleases; + for (i = 0; i < nleases; i++) + make_dhcp_lease(ret->leases.leases_val + i, leases[i]);
make_dhcp_lease can fail due to OOM; if it does, you ought to gracefully recover.
+ + mac = args->mac; + + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac,
You could just directly use args->mac here instead of going through the effort of declaring a local variable 'mac' (probably leftovers from trying to use remote_string instead of remote_nonnull_string in an earlier patch version).
+ if (leases && nleases) { + if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + goto cleanup; + + ret->leases.leases_len = nleases; + for (i = 0; i < nleases; i++) + make_dhcp_lease(ret->leases.leases_val + i, leases[i]);
Again, this should gracefully handle OOM.
+ +/* Copy contents of virNetworkDHCPLeasesPtr to remote_network_dhcp_lease */ +static void +make_dhcp_lease(remote_network_dhcp_lease *lease_dst, virNetworkDHCPLeasesPtr lease_src) +{ + char **hostname_tmp = NULL; + char **clientid_tmp = NULL; + ignore_value(VIR_ALLOC_QUIET(hostname_tmp)); + ignore_value(VIR_ALLOC_QUIET(clientid_tmp));
Rather than ignoring failure, we should clean up the intermediate object and return -1 to the caller, so that they can then clean up all the earlier array slots and propagate the error to the other end of RPC.
+ + lease_dst->expirytime = lease_src->expirytime; + lease_dst->type = lease_src->type; + lease_dst->prefix = lease_src->prefix; + if (lease_src->type == VIR_IP_ADDR_TYPE_IPV4) + ignore_value(VIR_STRDUP_QUIET(lease_dst->id.remote_interface_id_u.mac, + lease_src->id.mac)); + else + lease_dst->id.remote_interface_id_u.iaid = lease_src->id.iaid; + ignore_value(VIR_STRDUP_QUIET(lease_dst->ipaddr, lease_src->ipaddr)); + ignore_value(VIR_STRDUP_QUIET(*hostname_tmp, lease_src->hostname)); + ignore_value(VIR_STRDUP_QUIET(*clientid_tmp, lease_src->clientid));
Why two spaces after comma?
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 96ccb99..95910b6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -14,7 +14,7 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. * - * You should have received a copy of the GNU Lesser General Public + * You should have received a make of the GNU Lesser General Public
Whoops. Bogus commit hunk.
@@ -150,6 +150,7 @@ static void make_nonnull_storage_vol(remote_nonnull_storage_vol *vol_dst, virSto static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src); static void make_nonnull_nwfilter(remote_nonnull_nwfilter *nwfilter_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); +static void make_dhcp_lease(virNetworkDHCPLeasesPtr lease_src, remote_network_dhcp_lease *lease_dst);
Again, we shouldn't be copying bad practice. Make this function return int, and consider rearranging the file so that the helper function is implemented first without needing a forward declaration (see remoteSerializeTypedParameters as an example of a function that did a bit better job of being declared in order and returning OOM failure).
static void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event); /*----------------------------------------------------------------------*/
@@ -1058,7 +1059,7 @@ doRemoteClose(virConnectPtr conn, struct private_data *priv) virObjectUnref(priv->qemuProgram); priv->remoteProgram = priv->qemuProgram = priv->lxcProgram = NULL;
- /* Free hostname copy */ + /* Free hostname make */
Another bogus hunk. Did you do some sort of global search-and-replace s/copy/make/?
VIR_FREE(priv->hostname);
/* See comment for remoteType. */ @@ -3756,7 +3757,7 @@ static sasl_callback_t *remoteAuthMakeCallbacks(int *credtype, int ncredtype) * * Builds up an array of libvirt credential structs, populating * with data from the SASL interaction struct. These two structs - * are basically a 1-to-1 copy of each other. + * are basically a 1-to-1 make of each other.
Bogus.
+static int +remoteNetworkGetDHCPLeases(virNetworkPtr net, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1;
+ if (leases) { + if (ret.leases.leases_len && + VIR_ALLOC_N(leases_ret, ret.leases.leases_len + 1) < 0) { + goto cleanup; + } + + for (i = 0; i < ret.leases.leases_len; i++) { + if (VIR_ALLOC(leases_ret[i]) < 0) + goto cleanup; + + make_dhcp_lease(leases_ret[i], &ret.leases.leases_val[i]);
Need to check for OOM failure in make_dhcp_lease.
@@ -7021,6 +7182,8 @@ static virNetworkDriver network_driver = { .networkSetAutostart = remoteNetworkSetAutostart, /* 0.3.0 */ .networkIsActive = remoteNetworkIsActive, /* 0.7.3 */ .networkIsPersistent = remoteNetworkIsPersistent, /* 0.7.3 */ + .networkGetDHCPLeases = remoteNetworkGetDHCPLeases, /* 1.1.3 */ + .networkGetDHCPLeasesForMAC = remoteNetworkGetDHCPLeasesForMAC, /* 1.1.3 */
This will need to be bumped to 1.1.4. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
By querying the driver for the path of the leases file for the given virtual network and parsing it to retrieve info. src/network/bridge_driver.c: * Implement networkGetDHCPLeases * Implement networkGetDHCPLeasesForMAC * Implement networkGetDHCPLeasesHelper --- src/network/bridge_driver.c | 240 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 240 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8787bdb..e1c97c7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -109,6 +109,29 @@ static int networkPlugBandwidth(virNetworkObjPtr net, virDomainNetDefPtr iface); static int networkUnplugBandwidth(virNetworkObjPtr net, virDomainNetDefPtr iface); +/** + * VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX: + * + * Macro providing the upper limit on the size of leases file + */ +#define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX 2097152 + +/** + * VIR_NETWORK_DHCP_LEASE_FIELDS: + * + * Macro providing the maximum number of fields in an entry in + * the leases file + */ +#define VIR_NETWORK_DHCP_LEASE_FIELDS 5 + +/** + * VIR_NETWORK_DHCP_LEASE_SEPARATOR_FIELDS: + * + * Macro providing the maximum number of fields in the separator + * line in the DHCPv6 leases file + */ +#define VIR_NETWORK_DHCP_LEASE_SEPARATOR_FIELDS 2 + static virNetworkDriverStatePtr driverState = NULL; @@ -2988,6 +3011,221 @@ cleanup: return ret; } +/* This function parses the leases file of dnsmasq. + * + * An example of DHCPv4 leases file content: + * + * 1379024255 52:54:00:20:70:3d 192.168.105.240 * * + * 1379023351 52:54:00:b1:70:19 192.168.105.201 * * + * + * An example of DHCPv6 leases file content: + * + * 1380150262 52:54:00:2e:1d:22 192.168.122.80 * * + * 1380152517 52:54:00:72:0f:1e 192.168.122.119 * * + * duid 00:01:00:01:19:d3:ec:62:f0:4d:a2:8c:14:51 + * 1380152445 3022114 2001:db8:ca2:2:1::92 * 00:01:00:01:19:d6:0c:59:52:54:00:2e:1d:22 + * 1380152453 7474974 2001:db8:ca2:2:1::4e * 00:01:00:01:19:d5:e0:86:52:54:00:72:0f:1e + * + */ +static int +networkGetDHCPLeasesHelper(virNetworkPtr network, + virNetworkObjPtr obj, + const char *mac, + virNetworkDHCPLeasesPtr **leases) +{ + int rv = -1; + size_t i = 0; + size_t nleases = 0; + char *lease_file = NULL; + char **lease_fields = NULL; + char *lease_entries = NULL; + char *lease_entry = NULL; + virNetworkDHCPLeasesPtr *leases_ret = NULL; + virNetworkDHCPLeasesPtr lease = NULL; + int lease_file_len = 0; + bool ipv6 = false; + bool need_results = !!leases; + + /* Retrieve leases file location */ + lease_file = networkDnsmasqLeaseFileNameDefault(network->name); + + if ((lease_file_len = virFileReadAll(lease_file, + VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, + &lease_entries)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to read leases file: %s"), lease_file); + goto cleanup; + } + + lease_entry = lease_entries[0] == '\0' ? NULL : lease_entries; + + while (lease_entry) { + int nfields = 0; + + char *eol = strchr(lease_entry, '\n'); + *eol = '\0'; + + /* Split the lease line */ + if (!(lease_fields = virStringSplit(lease_entry, " ", + VIR_NETWORK_DHCP_LEASE_FIELDS))) + goto error; + + nfields = virStringListLength(lease_fields); + + /* Forward lease_entry to the next lease */ + lease_entry = strchr(lease_entry, '\0'); + if (lease_entry - lease_entries + 1 < lease_file_len) + lease_entry++; + else + lease_entry = NULL; + + /* Ignore the separator line in case of DHCPv6 */ + if (nfields == VIR_NETWORK_DHCP_LEASE_SEPARATOR_FIELDS + && STREQ(lease_fields[0], "duid")) { + ipv6 = true; + virStringFreeList(lease_fields); + continue; + } + + if (nfields != VIR_NETWORK_DHCP_LEASE_FIELDS) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of lease params aren't equal to: %d"), + VIR_NETWORK_DHCP_LEASE_FIELDS); + goto error; + } + + if (mac && virMacAddrCompare(mac, lease_fields[1])) { + virStringFreeList(lease_fields); + continue; + } + + if (need_results) { + if (VIR_ALLOC(lease) < 0) + goto error; + + /* Convert expirytime here */ + if (virStrToLong_ll(lease_fields[0], NULL, 10, &(lease->expirytime)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to convert lease expiry time to integer: %s"), + lease_fields[0]); + goto error; + } + + lease->type = ipv6 ? VIR_IP_ADDR_TYPE_IPV6 : VIR_IP_ADDR_TYPE_IPV4; + lease->prefix = virSocketAddrGetIpPrefix(&obj->def->ips->address, + &obj->def->ips->netmask, + obj->def->ips->prefix); + + if ((VIR_STRDUP(lease->id.mac, lease_fields[1]) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_fields[2]) < 0)) + goto error; + + /* Only two fields, hostname and clientid can be NULL + * Refer: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2013q3/007544.html + */ + if (STREQ(lease_fields[3], "*")) + lease->hostname = NULL; + else if (VIR_STRDUP(lease->hostname, lease_fields[3]) < 0) + goto error; + + if (STREQ(lease_fields[4], "*")) + lease->clientid = NULL; + else if (VIR_STRDUP(lease->clientid, lease_fields[4]) < 0) + goto error; + + if (VIR_INSERT_ELEMENT(leases_ret, nleases, nleases, lease) < 0) + goto error; + } + else + nleases++; + + VIR_FREE(lease); + virStringFreeList(lease_fields); + } + + lease_fields = NULL; + + if (need_results && mac && !leases_ret) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no lease with matching MAC address: %s"), mac); + goto error; + } + + if (leases_ret) { + /* NULL terminated array */ + ignore_value(VIR_REALLOC_N(leases_ret, nleases + 1)); + *leases = leases_ret; + leases_ret = NULL; + } + + rv = nleases; + +cleanup: + if (lease) + VIR_FREE(lease); + VIR_FREE(lease_file); + VIR_FREE(lease_entries); + if (lease_fields) + virStringFreeList(lease_fields); + return rv; + +error: + if (leases_ret) { + for (i = 0; i < nleases; i++) + virNetworkDHCPLeaseFree(leases_ret[i]); + VIR_FREE(leases_ret); + } + goto cleanup; +} + +static int +networkGetDHCPLeases(virNetworkPtr network, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + virNetworkObjPtr obj; + + virCheckFlags(0, -1); + + if (!(obj = networkObjFromNetwork(network))) + return rv; + + if (virNetworkGetDHCPLeasesEnsureACL(network->conn, obj->def) < 0) + goto cleanup; + + rv = networkGetDHCPLeasesHelper(network, obj, NULL, leases); + +cleanup: + if (obj) + virNetworkObjUnlock(obj); + return rv; +} + +static int +networkGetDHCPLeasesForMAC(virNetworkPtr network, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + virNetworkObjPtr obj; + + virCheckFlags(0, -1); + + if (!(obj = networkObjFromNetwork(network))) + return rv; + + if (virNetworkGetDHCPLeasesForMACEnsureACL(network->conn, obj->def) < 0) + goto cleanup; + + rv = networkGetDHCPLeasesHelper(network, obj, mac, leases); + +cleanup: + if (obj) + virNetworkObjUnlock(obj); + return rv; +} static virNetworkDriver networkDriver = { "Network", @@ -3012,6 +3250,8 @@ static virNetworkDriver networkDriver = { .networkSetAutostart = networkSetAutostart, /* 0.2.1 */ .networkIsActive = networkIsActive, /* 0.7.3 */ .networkIsPersistent = networkIsPersistent, /* 0.7.3 */ + .networkGetDHCPLeases = networkGetDHCPLeases, /* 1.1.3 */ + .networkGetDHCPLeasesForMAC = networkGetDHCPLeasesForMAC, /* 1.1.3 */ }; static virStateDriver networkStateDriver = { -- 1.7.11.7
Removing redundant if, diff attached. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
Rectified attachment. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
Use virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC in virsh. The new feature supports the follwing methods: 1. Retrieve leases info for a given virtual network 2. Retrieve leases info for given network interface tools/virsh-domain-monitor.c * Introduce new command : net-dhcp-leases Example Usage: net-dhcp-leases <network> [mac] virsh # net-dhcp-leases default6 Expiry Time MAC address / IAID Protocol IP address Hostname Client ID / DUID ------------------------------------------------------------------------------------------------------------------- 2013-09-26 12:46:15 140483776435424 ipv6 2001:db8:ca2:2:1::92/24 (null) 00:01:00:01:19:d6:0c:59:52:54:00:2e:1d:22 2013-09-26 12:26:29 140483776438128 ipv6 2001:db8:ca2:2:1::4e/24 (null) 00:01:00:01:19:d5:e0:86:52:54:00:72:0f:1e 2013-09-26 12:34:31 52:54:00:2e:1d:22 ipv4 192.168.122.80/24 (null) (null) 2013-09-26 12:13:24 52:54:00:72:0f:1e ipv4 192.168.122.119/24 (null) (null) tools/virsh.pod * Document new command --- tools/virsh-network.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 6 ++ 2 files changed, 156 insertions(+) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 8ddd5ca..5786ca2 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1129,6 +1129,150 @@ cmdNetworkEdit(vshControl *ctl, const vshCmd *cmd) return ret; } +/* + * "net-dhcp-leases" command + */ +static const vshCmdInfo info_network_dhcp_leases[] = { + {.name = "help", + .data = N_("print lease info for a given network") + }, + {.name = "desc", + .data = N_("Print lease info for a given network") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_network_dhcp_leases[] = { + {.name = "network", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("network name or uuid") + }, + {.name = "mac", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_NONE, + .help = N_("MAC address") + }, + {.name = NULL} +}; + +static int +vshNetworkDHCPLeaseSorter(const void *a, const void *b) +{ + char *id1 = NULL; + char *id2 = NULL; + int rv = -1; + + virNetworkDHCPLeasesPtr *lease1 = (virNetworkDHCPLeasesPtr *) a; + virNetworkDHCPLeasesPtr *lease2 = (virNetworkDHCPLeasesPtr *) b; + + if (*lease1 && !*lease2) + return -1; + + if (!*lease1) + return *lease2 != NULL; + + + switch ((*lease1)->type) { + case VIR_IP_ADDR_TYPE_IPV4: + ignore_value(virAsprintf(&id1, "%s", (*lease1)->id.mac)); + break; + case VIR_IP_ADDR_TYPE_IPV6: + ignore_value(virAsprintf(&id1, "%lu", (*lease1)->id.iaid)); + break; + } + switch ((*lease2)->type) { + case VIR_IP_ADDR_TYPE_IPV4: + ignore_value(virAsprintf(&id2, "%s", (*lease2)->id.mac)); + break; + case VIR_IP_ADDR_TYPE_IPV6: + ignore_value(virAsprintf(&id2, "%lu", (*lease2)->id.iaid)); + break; + } + + rv = vshStrcasecmp(id1, id2); + VIR_FREE(id1); + VIR_FREE(id2); + return rv; +} + +static bool +cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) +{ + const char *name = NULL; + const char *mac = NULL; + virNetworkDHCPLeasesPtr *leases = NULL; + int nleases = 0; + bool ret = false; + size_t i; + unsigned int flags = 0; + virNetworkPtr network = NULL; + + if (vshCommandOptString(cmd, "mac", &mac) < 0) + return false; + + if (!(network = vshCommandOptNetwork(ctl, cmd, &name))) + return false; + + nleases = mac ? virNetworkGetDHCPLeasesForMAC(network, mac, &leases, flags) + : virNetworkGetDHCPLeases(network, &leases, flags); + + if (nleases < 0) { + vshError(ctl, _("Failed to get leases info for %s"), name); + goto cleanup; + } + + /* Sort the list according to MAC Address/IAID */ + qsort(leases, nleases, sizeof(*leases), vshNetworkDHCPLeaseSorter); + + vshPrintExtra(ctl, " %-20s %-25s %-10s %-25s %-12s %s\n%s%s\n", + _("Expiry Time"), _("MAC address / IAID"), _("Protocol"), + _("IP address"), _("Hostname"), _("Client ID / DUID"), + "----------------------------------------------------------", + "---------------------------------------------------------"); + + for (i = 0; i < nleases; i++) { + const char *type = NULL; + char *id = NULL; + char *cidr_format = NULL; + virNetworkDHCPLeasesPtr lease = leases[i]; + time_t expirytime_tmp = lease->expirytime; + struct tm ts; + char expirytime[32]; + ts = *localtime_r(&expirytime_tmp, &ts); + strftime(expirytime, sizeof(expirytime), "%Y-%m-%d %H:%M:%S", &ts); + + switch (lease->type) { + case VIR_IP_ADDR_TYPE_IPV4: + type = "ipv4"; + ignore_value(virAsprintf(&id, "%s", lease->id.mac)); + break; + case VIR_IP_ADDR_TYPE_IPV6: + type = "ipv6"; + ignore_value(virAsprintf(&id, "%lu", lease->id.iaid)); + break; + } + + ignore_value(virAsprintf(&cidr_format, "%s/%d", + lease->ipaddr, lease->prefix)); + + vshPrintExtra(ctl, " %-20s %-25s %-10s %-25s %-12s %s\n", + expirytime, NULLSTR(id), NULLSTR(type), cidr_format, + NULLSTR(lease->hostname), NULLSTR(lease->clientid)); + VIR_FREE(id); + } + + ret = true; + +cleanup: + if (leases) { + for (i = 0; i < nleases; i++) + virNetworkDHCPLeaseFree(leases[i]); + VIR_FREE(leases); + } + virNetworkFree(network); + return ret; +} const vshCmdDef networkCmds[] = { {.name = "net-autostart", @@ -1209,5 +1353,11 @@ const vshCmdDef networkCmds[] = { .info = info_network_uuid, .flags = 0 }, + {.name = "net-dhcp-leases", + .handler = cmdNetworkDHCPLeases, + .opts = opts_network_dhcp_leases, + .info = info_network_dhcp_leases, + .flags = 0, + }, {.name = NULL} }; diff --git a/tools/virsh.pod b/tools/virsh.pod index e12a800..ed5fc62 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2333,6 +2333,12 @@ If I<--current> is specified, affect the current network state. Both I<--live> and I<--config> flags may be given, but I<--current> is exclusive. Not specifying any flag is the same as specifying I<--current>. +=item B<net-dhcp-leases> I<network> [I<mac>] + +Get a list of dhcp leases for all network interfaces connected to the given +virtual I<network> or limited output just for one interface if I<mac> is +specified. + =back =head1 INTERFACE COMMANDS -- 1.7.11.7
In accordance with latest rule: use VIR_STRDUP instead of virAsprintf with "%s" Diff attached. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
Rectified attachment. -- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Nehal J Wani