[libvirt] [PATCHv3 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 v3 * Mostly small nits, change in MACRO names, use of virSocketAddrGetIpPrefix to retrieve IP prefix from @dom XML. 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 public APIs net-dhcp-leases: Implement the remote protocol net-dhcp-leases: Private implementation inside network driver net-dhcp-leases: Add virsh support daemon/remote.c | 133 ++++++++++++++++++++++++++++++++ include/libvirt/libvirt.h.in | 33 ++++++++ python/generator.py | 3 + src/driver.h | 13 ++++ src/libvirt.c | 173 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 7 ++ src/network/bridge_driver.c | 177 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 150 ++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 50 +++++++++++- src/remote_protocol-structs | 32 ++++++++ src/rpc/gendispatch.pl | 1 + tools/virsh-network.c | 100 ++++++++++++++++++++++++ tools/virsh.pod | 6 ++ 13 files changed, 877 insertions(+), 1 deletion(-) -- 1.7.11.7
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree. * virNetworkGetDHCPLeases: returns the dhcp leases information for a given virtual network. The information includes lease expirytime, MAC Address, IP Address (with type and prefix), hostname and clientid. * 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 | 33 +++++++++ python/generator.py | 3 + src/driver.h | 13 ++++ src/libvirt.c | 173 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 7 ++ 5 files changed, 229 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a47e33c..d92a720 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2800,6 +2800,39 @@ 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; + char *mac; /* MAC address */ + char *ipaddr; /* IP address */ + char *hostname; /* Hostname */ + char *clientid; /* Client ID */ + 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 a91dde8..e76cbfc 100755 --- a/python/generator.py +++ b/python/generator.py @@ -460,6 +460,8 @@ skip_impl = ( 'virNodeGetCPUMap', 'virDomainMigrate3', 'virDomainMigrateToURI3', + 'virNetworkGetDHCPLeases', + 'virNetworkGetDHCPLeasesForMAC', ) lxc_skip_impl = ( @@ -560,6 +562,7 @@ skip_function = ( "virTypedParamsGetString", "virTypedParamsGetUInt", "virTypedParamsGetULLong", + 'virNetworkDHCPLeaseFree', ) lxc_skip_function = ( diff --git a/src/driver.h b/src/driver.h index be64333..698e0ca 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1121,6 +1121,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; @@ -1451,6 +1462,8 @@ struct _virNetworkDriver { virDrvNetworkSetAutostart networkSetAutostart; virDrvNetworkIsActive networkIsActive; virDrvNetworkIsPersistent networkIsPersistent; + virDrvNetworkGetDHCPLeases networkGetDHCPLeases; + virDrvNetworkGetDHCPLeasesForMAC networkGetDHCPLeasesForMAC; }; diff --git a/src/libvirt.c b/src/libvirt.c index a6fcab0..05d20e9 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" @@ -21966,3 +21967,175 @@ error: virDispatchError(dom->conn); return -1; } + +/** + * virNetworkGetDHCPLeases: + * @network: pointer to network object + * @leases: pointer to an array of pointers pointing to the obtained leases + * @flags: extra flags, not used yet, so callers should always pass 0 + * + * The API returns lease info for all network interfaces connected to the + * given virtual network. + * + * 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); + * } + * + * if (leases) { + * for (i = 0; i < nleases; i++) + * 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(); + + virCheckNonNullArgGoto(network, error); + + 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: MAC Address of an interface + * @leases: pointer to an array of pointers pointing to the obtained leases + * @flags: extra flags, not used yet, so callers should always pass 0 + * + * The API returns the leases info of the interface which matches with the + * given @mac. There can be multiple leases for a single @mac because this + * API intends to support 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(); + + virCheckNonNullArgGoto(network, error); + virCheckNonNullArgGoto(mac, error); + + if (leases) + *leases = NULL; + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (mac && virMacAddrParse(mac, &addr) < 0) { + virReportInvalidArg(mac, + _("Given MAC Address doesn't comply " + "with the standard (IEEE 802) format in %s"), + __FUNCTION__); + 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; + VIR_FREE(lease->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 bbdf78a..e952869 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -634,4 +634,11 @@ LIBVIRT_1.1.1 { virDomainSetMemoryStatsPeriod; } LIBVIRT_1.1.0; +LIBVIRT_1.1.3 { + global: + virNetworkGetDHCPLeases; + virNetworkGetDHCPLeasesForMAC; + virNetworkDHCPLeaseFree; +} LIBVIRT_1.1.1; + # .... define new API here using predicted next version number .... -- 1.7.11.7
On Mon, Sep 16, 2013 at 11:19:13AM +0530, Nehal J Wani wrote:
+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(); + + virCheckNonNullArgGoto(network, error); + virCheckNonNullArgGoto(mac, error); + + if (leases) + *leases = NULL; + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (mac && virMacAddrParse(mac, &addr) < 0) { + virReportInvalidArg(mac, + _("Given MAC Address doesn't comply " + "with the standard (IEEE 802) format in %s"), + __FUNCTION__);
Don't pass __FUNCTION__ in this error message - that is already done automatically
+ goto error; + }
'mac' should be a mandatory parameter here.
+ + 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; +}
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 Mon, Sep 16, 2013 at 3:22 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Sep 16, 2013 at 11:19:13AM +0530, Nehal J Wani wrote:
+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(); + + virCheckNonNullArgGoto(network, error); + virCheckNonNullArgGoto(mac, error); + + if (leases) + *leases = NULL; + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (mac && virMacAddrParse(mac, &addr) < 0) { + virReportInvalidArg(mac, + _("Given MAC Address doesn't comply " + "with the standard (IEEE 802) format in %s"), + __FUNCTION__);
Don't pass __FUNCTION__ in this error message - that is already done automatically
+ goto error; + }
'mac' should be a mandatory parameter here.
Attached diff should fix it. But there are still calls to virReportInvalidArg() with __FUNCTION__ as one of its arguments in src/libvirt.c
+ + 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; +}
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
PFA: Diff On Mon, Sep 16, 2013 at 8:44 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
On Mon, Sep 16, 2013 at 3:22 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Sep 16, 2013 at 11:19:13AM +0530, Nehal J Wani wrote:
+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(); + + virCheckNonNullArgGoto(network, error); + virCheckNonNullArgGoto(mac, error); + + if (leases) + *leases = NULL; + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (mac && virMacAddrParse(mac, &addr) < 0) { + virReportInvalidArg(mac, + _("Given MAC Address doesn't comply " + "with the standard (IEEE 802) format in %s"), + __FUNCTION__);
Don't pass __FUNCTION__ in this error message - that is already done automatically
+ goto error; + }
'mac' should be a mandatory parameter here.
Attached diff should fix it.
But there are still calls to virReportInvalidArg() with __FUNCTION__ as one of its arguments in src/libvirt.c
+ + 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; +}
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
-- Nehal J Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad http://commandlinewani.blogspot.com
On Mon, Sep 16, 2013 at 08:45:09PM +0530, Nehal J Wani wrote:
PFA: Diff
On Mon, Sep 16, 2013 at 8:44 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
On Mon, Sep 16, 2013 at 3:22 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Sep 16, 2013 at 11:19:13AM +0530, Nehal J Wani wrote:
+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(); + + virCheckNonNullArgGoto(network, error); + virCheckNonNullArgGoto(mac, error); + + if (leases) + *leases = NULL; + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (mac && virMacAddrParse(mac, &addr) < 0) { + virReportInvalidArg(mac, + _("Given MAC Address doesn't comply " + "with the standard (IEEE 802) format in %s"), + __FUNCTION__);
Don't pass __FUNCTION__ in this error message - that is already done automatically
+ goto error; + }
'mac' should be a mandatory parameter here.
Attached diff should fix it.
No, it makes it worse - you're now allowing a NULL mac parameter to be passed to virMacAddrParse which will cause a segv. You need to be checking for mac == NULL with virCheckNonNullArg 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 Mon, Sep 16, 2013 at 8:55 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Sep 16, 2013 at 08:45:09PM +0530, Nehal J Wani wrote:
PFA: Diff
On Mon, Sep 16, 2013 at 8:44 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
On Mon, Sep 16, 2013 at 3:22 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Sep 16, 2013 at 11:19:13AM +0530, Nehal J Wani wrote:
+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(); + + virCheckNonNullArgGoto(network, error); + virCheckNonNullArgGoto(mac, error); + + if (leases) + *leases = NULL; + + if (!VIR_IS_CONNECTED_NETWORK(network)) { + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (mac && virMacAddrParse(mac, &addr) < 0) { + virReportInvalidArg(mac, + _("Given MAC Address doesn't comply " + "with the standard (IEEE 802) format in %s"), + __FUNCTION__);
Don't pass __FUNCTION__ in this error message - that is already done automatically
+ goto error; + }
'mac' should be a mandatory parameter here.
Attached diff should fix it.
No, it makes it worse - you're now allowing a NULL mac parameter to be passed to virMacAddrParse which will cause a segv. You need to be checking for mac == NULL with virCheckNonNullArg
I think you missed this, I am already using virCheckNonNullArg : + virConnectPtr conn; + virMacAddr addr; + + VIR_DEBUG("network=%p, mac=%s, leases=%p, flags=%x", + network, mac, leases, flags); + + virResetLastError(); + + virCheckNonNullArgGoto(network, error); + virCheckNonNullArgGoto(mac, error); + + if (leases) + *leases = 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 09/15/2013 11:49 PM, Nehal J Wani wrote:
Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC and virNetworkDHCPLeaseFree.
+ +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases; +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr; +struct _virNetworkDHCPLeases { + long long expirytime;
In what unit? Seconds since Epoch?
+ +/** + * virNetworkGetDHCPLeases: + * @network: pointer to network object + * @leases: pointer to an array of pointers pointing to the obtained leases
Try to make it more clear that this is an output parameter. Copying from virConnectListAllDomains, I'd try something like: @leases: Pointer to a variable to store the array containing details on obtained leases, or NULL if the list is not requires (just returns number of leases)
+ * @flags: extra flags, not used yet, so callers should always pass 0 + * + * The API returns lease info for all network interfaces connected to the + * given virtual network. + * + * 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.
I'm not sure if writing virNetworkDHCPLeaseFree() (with the () at the end) will make cross-referencing any nicer in the generated html docs, but it can't hurt.
+ * + * 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);
If you stick 'virNetworkDHCPLeaseFree(lease);' here,...
+ * } + * + * if (leases) { + * for (i = 0; i < nleases; i++) + * virNetworkDHCPLeaseFree(leases[i]); + * }
...you can omit this loop entirely.
+ * free(leases); + * + */ +int +virNetworkGetDHCPLeases(virNetworkPtr network, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags)
+/** + * virNetworkGetDHCPLeasesForMAC: + * @network: pointer to network object + * @mac: MAC Address of an interface
Maybe make it clear that this is the stringized form: @mac: ASCII formatted MAC address of an interface
+ * @leases: pointer to an array of pointers pointing to the obtained leases
Same suggestion as above.
+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(); + + virCheckNonNullArgGoto(network, error);
This check is redundant...[1]
+ virCheckNonNullArgGoto(mac, error);
This check...[2]
+ + if (leases) + *leases = NULL;
[2]...should be moved after this, so that we guarantee to set *leases on ALL possible errors.
+ + if (!VIR_IS_CONNECTED_NETWORK(network)) {
[1]...with this. (Same problem with the other function, too).
+ virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + /* Validate the MAC address */ + if (mac && virMacAddrParse(mac, &addr) < 0) {
You already required non-NULL mac, so 'mac &&' is bogus.
+++ b/src/libvirt_public.syms @@ -634,4 +634,11 @@ LIBVIRT_1.1.1 { virDomainSetMemoryStatsPeriod; } LIBVIRT_1.1.0;
+LIBVIRT_1.1.3 { + global: + virNetworkGetDHCPLeases; + virNetworkGetDHCPLeasesForMAC; + virNetworkDHCPLeaseFree;
Not strictly required, but I like sorting symbols in this listing. -- 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 src/remote/remote_driver.c * Define remoteNetworkGetDHCPLeases * Define remoteNetworkGetDHCPLeasesForMAC 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 | 133 ++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 150 +++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 50 ++++++++++++++- src/remote_protocol-structs | 32 +++++++++ src/rpc/gendispatch.pl | 1 + 5 files changed, 365 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 2aff7c1..5252893 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5137,7 +5137,140 @@ cleanup: return rv; } +static int +remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases, + int nleases, + remote_network_get_dhcp_leases_ret *ret) +{ + size_t i; + + 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 (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + return -1; + + ret->leases.leases_len = nleases; + + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasesPtr lease = leases[i]; + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + lease_ret->expirytime = lease->expirytime; + lease_ret->type = lease->type; + lease_ret->prefix = lease->prefix; + + if ((VIR_STRDUP(lease_ret->mac, lease->mac) < 0) || + (VIR_STRDUP(lease_ret->ipaddr, lease->ipaddr) < 0) || + (VIR_STRDUP(lease_ret->hostname, lease->hostname) < 0) || + (VIR_STRDUP(lease_ret->clientid, lease->clientid) < 0)) + goto error; + } + + return 0; + +error: + for (i = 0; i < nleases; i++) { + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret); + } + return -1; +} + +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; + 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, &leases, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, ret) < 0) + goto cleanup; + rv = nleases; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + 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; + 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 ? *args->mac : NULL; + + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac, &leases, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, + (remote_network_get_dhcp_leases_ret *)ret) < 0) + goto cleanup; + + rv = nleases; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeaseFree(leases[i]); + } + } + VIR_FREE(leases); + virNetworkFree(net); + return rv; +} /*----- Helpers. -----*/ diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62e77a5..92ac8ef 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6599,6 +6599,154 @@ done: 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; + remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.mac = mac ? (char **)&mac : NULL; + args.flags = flags; + + 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); + return -1; + goto cleanup; + } + + 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; + + virNetworkDHCPLeasesPtr lease = leases_ret[i]; + remote_network_dhcp_lease *lease_ret = &(ret.leases.leases_val[i]); + lease->expirytime = lease_ret->expirytime; + lease->type = lease_ret->type; + lease->prefix = lease_ret->prefix; + + if ((VIR_STRDUP(lease->mac, lease_ret->mac) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_ret->ipaddr) < 0) || + (VIR_STRDUP(lease->hostname, lease_ret->hostname) < 0) || + (VIR_STRDUP(lease->clientid, lease_ret->clientid) < 0)) + goto cleanup; + } + + *leases = leases_ret; + leases_ret = NULL; + rv = ret.leases.leases_len; + +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 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; + remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.flags = flags; + + 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); + return -1; + goto cleanup; + } + + 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; + + virNetworkDHCPLeasesPtr lease = leases_ret[i]; + remote_network_dhcp_lease *lease_ret = &(ret.leases.leases_val[i]); + lease->expirytime = lease_ret->expirytime; + lease->type = lease_ret->type; + lease->prefix = lease_ret->prefix; + + if ((VIR_STRDUP(lease->mac, lease_ret->mac) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_ret->ipaddr) < 0) || + (VIR_STRDUP(lease->hostname, lease_ret->hostname) < 0) || + (VIR_STRDUP(lease->clientid, lease_ret->clientid) < 0)) + goto cleanup; + } + + *leases = leases_ret; + leases_ret = NULL; + rv = ret.leases.leases_len; + +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 void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) { @@ -6958,6 +7106,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 85ad9ba..a29646b 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -232,6 +232,11 @@ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; /* Upper limit on number of job stats */ const REMOTE_DOMAIN_JOB_STATS_MAX = 16; +/* + * 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]; @@ -2835,6 +2840,35 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; }; +struct remote_network_dhcp_lease { + hyper expirytime; + remote_nonnull_string mac; + remote_nonnull_string ipaddr; + remote_nonnull_string hostname; + remote_nonnull_string clientid; + int type; + unsigned int prefix; +}; + +struct remote_network_get_dhcp_leases_args { + remote_nonnull_network net; + unsigned int flags; +}; + +struct remote_network_get_dhcp_leases_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>; +}; + +struct remote_network_get_dhcp_leases_for_mac_args { + remote_nonnull_network net; + remote_string mac; + unsigned int flags; +}; + +struct remote_network_get_dhcp_leases_for_mac_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -4998,5 +5032,19 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311 + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311, + + /** + * @generate: none + * @priority: high + * @acl: network:read + */ + REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 312, + + /** + * @generate: none + * @priority: high + * @acl: network:read + */ + REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC = 313 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 4e27aae..9b625e8 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2316,6 +2316,36 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_domain dom; remote_nonnull_string devAlias; }; +struct remote_network_dhcp_lease { + int64_t expirytime; + remote_nonnull_string mac; + remote_nonnull_string ipaddr; + remote_nonnull_string hostname; + remote_nonnull_string clientid; + int type; + u_int prefix; +}; +struct remote_network_get_dhcp_leases_args { + remote_nonnull_network net; + u_int flags; +}; +struct remote_network_get_dhcp_leases_ret { + struct { + u_int leases_len; + remote_network_dhcp_lease * leases_val; + } leases; +}; +struct remote_network_get_dhcp_leases_for_mac_args { + remote_nonnull_network net; + remote_string mac; + u_int flags; +}; +struct remote_network_get_dhcp_leases_for_mac_ret { + struct { + u_int leases_len; + remote_network_dhcp_lease * leases_val; + } leases; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2628,4 +2658,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_CREATE_XML_WITH_FILES = 309, REMOTE_PROC_DOMAIN_CREATE_WITH_FILES = 310, REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311, + REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 312, + REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC = 313, }; 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
+static int +remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases, + int nleases, + remote_network_get_dhcp_leases_ret *ret) +{ + size_t i; + + 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 (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + return -1; + + ret->leases.leases_len = nleases; + + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasesPtr lease = leases[i]; + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + lease_ret->expirytime = lease->expirytime; + lease_ret->type = lease->type; + lease_ret->prefix = lease->prefix; + + if ((VIR_STRDUP(lease_ret->mac, lease->mac) < 0) || + (VIR_STRDUP(lease_ret->ipaddr, lease->ipaddr) < 0) || + (VIR_STRDUP(lease_ret->hostname, lease->hostname) < 0) || + (VIR_STRDUP(lease_ret->clientid, lease->clientid) < 0)) + goto error; + } + + return 0; + +error: + for (i = 0; i < nleases; i++) { + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret);
+ } + return -1; +} + +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; + 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, &leases, args->flags)) <
[1] 0)
+ goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, ret) < 0) + goto cleanup;
+ rv = nleases; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + 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; + 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 ? *args->mac : NULL; + + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac, &leases, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, + (remote_network_get_dhcp_leases_ret *)ret) < 0) [2]
+ goto cleanup; + + rv = nleases; +
I am a little skeptical about the typecasts involved in [1] and [2] Specifically for [2], although the APIs are different, the struct is same, only the name is different. But, what would be the other options? One option: (Suggested by Osier) Change remoteSerializeNetworkDHCPLeases to +remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases, + int nleases, + (void *) ret, + int method) where the datatype for variable in which ret will be copied, will be decided based on the method (method's value will be taken from an enum or something similar, e.g: 1->DHCPLeases, 2->DHCPLeasesForMAC) OR Second Option: Since the APIs should be independent, make another function: remoteSerializeNetworkDHCPLeasesForMAC(...), but that will lead to code redundancy. Eric, what would you do in these cases?
On Mon, Sep 16, 2013 at 11:31:27PM +0530, Nehal J Wani wrote:
+static int +remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases, + int nleases, + remote_network_get_dhcp_leases_ret *ret) +{ + size_t i; + + if (nleases > REMOTE_NETWORK_DHCP_LEASES_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of leases is %d, which exceeds max limit: %d"),
Please make your email client not mangle line breaks when you are replying. It makes it really hard to follow quoted text.
+ nleases, REMOTE_NETWORK_DHCP_LEASES_MAX); + return -1; + } + + if (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + return -1; + + ret->leases.leases_len = nleases; + + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasesPtr lease = leases[i]; + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + lease_ret->expirytime = lease->expirytime; + lease_ret->type = lease->type; + lease_ret->prefix = lease->prefix; + + if ((VIR_STRDUP(lease_ret->mac, lease->mac) < 0) || + (VIR_STRDUP(lease_ret->ipaddr, lease->ipaddr) < 0) || + (VIR_STRDUP(lease_ret->hostname, lease->hostname) < 0) || + (VIR_STRDUP(lease_ret->clientid, lease->clientid) < 0)) + goto error; + } + + return 0; + +error: + for (i = 0; i < nleases; i++) { + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret);
[1]
+ goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, ret) < 0) + goto cleanup;
+ rv = nleases; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + 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; + 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 ? *args->mac : NULL; + + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac, &leases, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, + (remote_network_get_dhcp_leases_ret *)ret) < 0) [2]
+ } + return -1; +} + +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; + 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, &leases, args->flags)) <
+ goto cleanup; + + rv = nleases; +
I am a little skeptical about the typecasts involved in [1] and [2]
Specifically for [2], although the APIs are different, the struct is same, only the name is different. But, what would be the other options?
One option: (Suggested by Osier) Change remoteSerializeNetworkDHCPLeases to
+remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases, + int nleases, + (void *) ret, + int method)
where the datatype for variable in which ret will be copied, will be decided based on the method (method's value will be taken from an enum or something similar, e.g: 1->DHCPLeases, 2->DHCPLeasesForMAC)
OR
Second Option: Since the APIs should be independent, make another function: remoteSerializeNetworkDHCPLeasesForMAC(...), but that will lead to code redundancy.
The commonality between the methods is the 'remote_network_dhcp_lease' struct. So instead of creating a remoteSerializeNetworkDHCPLeases which serializes a list of leases. Make a helper method which just serializes a single "remote_network_dhcp_lease" instance. Push the list iteration code up into the parent methods. 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 09/16/2013 12:01 PM, Nehal J Wani wrote:
+ + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasesPtr lease = leases[i]; + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]);
+error: + for (i = 0; i < nleases; i++) { + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret);
[1]
Don't do that. lease_ret is NOT an abi-compatible struct with virNetworkDHCPLeasesPtr. You'll have to manually free the individual components, or use xdr_free(). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/15/2013 11:49 PM, Nehal J Wani wrote:
Implement RPC calls for virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC
A lot of my comments below are similar to what I did on Guiseppe's patch; you may want to closely study: https://www.redhat.com/archives/libvir-list/2013-September/msg00808.html
diff --git a/daemon/remote.c b/daemon/remote.c index 2aff7c1..5252893 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5137,7 +5137,140 @@ cleanup: return rv; }
+static int
Lately, we've been preferring a style of 2 blank lines between functions.
+remoteSerializeNetworkDHCPLeases(virNetworkDHCPLeasesPtr *leases, + int nleases, + remote_network_get_dhcp_leases_ret *ret) +{ + size_t i; + + 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 (VIR_ALLOC_N(ret->leases.leases_val, nleases) < 0) + return -1; + + ret->leases.leases_len = nleases;
I wouldn't set this...
+ + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeasesPtr lease = leases[i]; + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + lease_ret->expirytime = lease->expirytime; + lease_ret->type = lease->type; + lease_ret->prefix = lease->prefix; + + if ((VIR_STRDUP(lease_ret->mac, lease->mac) < 0) || + (VIR_STRDUP(lease_ret->ipaddr, lease->ipaddr) < 0) || + (VIR_STRDUP(lease_ret->hostname, lease->hostname) < 0) || + (VIR_STRDUP(lease_ret->clientid, lease->clientid) < 0)) + goto error; + }
...until here...
+ + return 0; + +error: + for (i = 0; i < nleases; i++) { + remote_network_dhcp_lease *lease_ret = &(ret->leases.leases_val[i]); + virNetworkDHCPLeaseFree((virNetworkDHCPLeasesPtr)lease_ret); + }
...so that the caller doesn't try to iterate over bogus remote_network_dhcp_lease data on error.
+ return -1; +} + +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; + 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, &leases, args->flags)) < 0)
This doesn't allow a user to efficiently request count-only semantics. In comparison with our ListAll* apis, doing that would require the use of an args->need_results member, where you can pass NULL instead of &leases if the user doesn't care about the results.
+ goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, ret) < 0) + goto cleanup;
+ rv = nleases;
The odd spacing of this non-patched blank line in the middle is the result of you not using 2 blanks between functions. rv must be 0, not positive, on success; the RPC client can reconstruct nleases from ret->leases.leases_val. Then again, if you plan on supporting NULL leases for count-only operation, you need to add ret->ret as a way to pass the count in the absence of a return array.
+ +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + 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; + 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 ? *args->mac : NULL;
args->mac should be non-NULL (since you already reject NULL at the API level).
+ + if ((nleases = virNetworkGetDHCPLeasesForMAC(net, mac, &leases, args->flags)) < 0) + goto cleanup; + + if (remoteSerializeNetworkDHCPLeases(leases, nleases, + (remote_network_get_dhcp_leases_ret *)ret) < 0) + goto cleanup; + + rv = nleases;
rv must be 0 on success.
+ +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (leases) { + size_t i; + for (i = 0; i < nleases; i++) { + virNetworkDHCPLeaseFree(leases[i]); + } + } + VIR_FREE(leases); + virNetworkFree(net); + return rv; +}
/*----- Helpers. -----*/
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62e77a5..92ac8ef 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6599,6 +6599,154 @@ done: return rv; }
+static int +remoteNetworkGetDHCPLeasesForMAC(virNetworkPtr net, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{
Why is the ForMAC version first here, but last in all the other hunks where you add the two functions?
+ 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;
Uninitialized...
+ remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.mac = mac ? (char **)&mac : NULL;
Again, I think args.mac is typed incorrectly. You should support args.need_results to allow passing a NULL leases through to the driver.
+ args.flags = flags; + + 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); + return -1; + goto cleanup;
Dead goto. The return -1 is wrong. With that removed, ...leases_ret is still uninitialized...
+ } + + if (ret.leases.leases_len &&
No need to populate leases_ret if the user passes leases==NULL.
+ 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; + + virNetworkDHCPLeasesPtr lease = leases_ret[i]; + remote_network_dhcp_lease *lease_ret = &(ret.leases.leases_val[i]); + lease->expirytime = lease_ret->expirytime; + lease->type = lease_ret->type; + lease->prefix = lease_ret->prefix; + + if ((VIR_STRDUP(lease->mac, lease_ret->mac) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_ret->ipaddr) < 0) || + (VIR_STRDUP(lease->hostname, lease_ret->hostname) < 0) || + (VIR_STRDUP(lease->clientid, lease_ret->clientid) < 0)) + goto cleanup;
I'm not sure if it's worth some transfer semantics instead of VIR_STRDUP, as in: lease->mac = lease_ret->mac; lease_ret->mac = NULL; On the one hand, it avoids some malloc pressure; on the other, it's a little bit harder to follow all paths to make sure there are no leaks or double frees.
+ } + + *leases = leases_ret; + leases_ret = NULL; + rv = ret.leases.leases_len; + +cleanup: + if (leases_ret) { + for (i = 0; i < ret.leases.leases_len; i++) + virNetworkDHCPLeaseFree(leases_ret[i]);
...and you dereference an uninit variable to start freeing random data. Hello SIGSEGV.
+ } + 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 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;
Same SEGSEGV on uninit data if something else fails.
+ remoteDriverLock(priv); + + make_nonnull_network(&args.net, net); + args.flags = flags; + + 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); + return -1; + goto cleanup;
Same dead goto.
+ } + + 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; + + virNetworkDHCPLeasesPtr lease = leases_ret[i]; + remote_network_dhcp_lease *lease_ret = &(ret.leases.leases_val[i]); + lease->expirytime = lease_ret->expirytime; + lease->type = lease_ret->type; + lease->prefix = lease_ret->prefix; + + if ((VIR_STRDUP(lease->mac, lease_ret->mac) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_ret->ipaddr) < 0) || + (VIR_STRDUP(lease->hostname, lease_ret->hostname) < 0) || + (VIR_STRDUP(lease->clientid, lease_ret->clientid) < 0)) + goto cleanup; + }
Same issue about not handling user's NULL leases.
+ + *leases = leases_ret; + leases_ret = NULL; + rv = ret.leases.leases_len; + +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 void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) { @@ -6958,6 +7106,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 85ad9ba..a29646b 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -232,6 +232,11 @@ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; /* Upper limit on number of job stats */ const REMOTE_DOMAIN_JOB_STATS_MAX = 16;
+/* + * 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];
@@ -2835,6 +2840,35 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; };
+struct remote_network_dhcp_lease { + hyper expirytime; + remote_nonnull_string mac; + remote_nonnull_string ipaddr; + remote_nonnull_string hostname; + remote_nonnull_string clientid; + int type; + unsigned int prefix; +}; + +struct remote_network_get_dhcp_leases_args { + remote_nonnull_network net; + unsigned int flags;
Needs a 'int need_results' member to pass NULL.
+}; + +struct remote_network_get_dhcp_leases_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>;
Needs a 'int ret' member to handle NULL leases.
+}; + +struct remote_network_get_dhcp_leases_for_mac_args { + remote_nonnull_network net; + remote_string mac;
This should be remote_nonnull_string (since the API already rejects NULL).
+ unsigned int flags; +}; + +struct remote_network_get_dhcp_leases_for_mac_ret { + remote_network_dhcp_lease leases<REMOTE_NETWORK_DHCP_LEASES_MAX>; +};
Same issues about handling NULL leases.
+ /*----- Protocol. -----*/
/* Define the program number, protocol version and procedure numbers here. */ @@ -4998,5 +5032,19 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311 + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311, + + /** + * @generate: none + * @priority: high
Is high priority appropriate? Is this something where we are always guaranteed that we will never need to block or interact with external processes to get the information, even if we change our implementation away from dnsmasq to something else?
+ * @acl: network:read + */ + REMOTE_PROC_NETWORK_GET_DHCP_LEASES = 312, + + /** + * @generate: none + * @priority: high + * @acl: network:read + */ + REMOTE_PROC_NETWORK_GET_DHCP_LEASES_FOR_MAC = 313 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 4e27aae..9b625e8 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs
Be sure to regenerate this after fixing the .x file. You may also have some (trivial) conflicts, depending on whether other new APIs go in first. -- 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 | 177 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3a8be90..f5adb97 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -109,6 +109,21 @@ static int networkPlugBandwidth(virNetworkObjPtr net, virDomainNetDefPtr iface); static int networkUnplugBandwidth(virNetworkObjPtr net, virDomainNetDefPtr iface); +/** + * VIR_NETWORK_DHCP_LEASE_LENGTH_MAX: + * + * Macro providing the maximum length of an entry in the leases file + * Refer: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2013q3/007402.html + */ +#define VIR_NETWORK_DHCP_LEASE_LENGTH_MAX 2048 + +/** + * VIR_NETWORK_DHCPLEASE_PARAMS: + * + * Macro providing the maximum number of parameters in an entry in + * the leases file + */ +#define VIR_NETWORK_DHCP_LEASE_FIELDS 5 static virNetworkDriverStatePtr driverState = NULL; @@ -2980,6 +2995,166 @@ cleanup: return ret; } +/* This function parese the leases file of dnsmasq. + * + * An example of 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 * * + */ +static int +networkGetDHCPLeasesHelper(virNetworkPtr network, + virNetworkObjPtr obj, + const char *mac, + virNetworkDHCPLeasesPtr **leases) +{ + int rv = -1; + size_t i = 0; + size_t nleases = 0; + char *leasefile; + char **lease_fields; + char dhcpentry[VIR_NETWORK_DHCP_LEASE_LENGTH_MAX]; + virNetworkDHCPLeasesPtr *leases_ret = NULL; + FILE *fp = NULL; + + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, "%s", + _("no network with matching name")); + return -1; + } + + /* Retrieve leases file location */ + leasefile = networkDnsmasqLeaseFileNameDefault(network->name); + if (!(fp = fopen(leasefile, "r"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to open leases file: %s"), leasefile); + goto cleanup; + } + + while (fgets(dhcpentry, sizeof(dhcpentry), fp) != NULL) { + virNetworkDHCPLeasesPtr lease = NULL; + + /* Remove newline */ + dhcpentry[strlen(dhcpentry) - 1] = '\0'; + + /* Split the lease line */ + lease_fields = virStringSplit(dhcpentry, " ", VIR_NETWORK_DHCP_LEASE_FIELDS); + + if (virStringListLength(lease_fields) != 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 && STRNEQ(mac, lease_fields[1])) + continue; + + if (VIR_EXPAND_N(leases_ret, nleases, 1) < 0) + goto error; + + if (VIR_ALLOC(leases_ret[nleases - 1]) < 0) + goto error; + + lease = leases_ret[nleases - 1]; + + /* 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; + } + + /* Hardcoded, as dnsmasq uses ipv4 */ + lease->type = VIR_IP_ADDR_TYPE_IPV4; + lease->prefix = virSocketAddrGetIpPrefix(&obj->def->ips->address, + &obj->def->ips->netmask, + obj->def->ips->prefix); + + if ((VIR_STRDUP(lease->mac, lease_fields[1]) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_fields[2]) < 0) || + (VIR_STRDUP(lease->hostname, lease_fields[3]) < 0) || + (VIR_STRDUP(lease->clientid, lease_fields[4]) < 0)) + goto error; + } + + if (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: + VIR_FORCE_FCLOSE(fp); + VIR_FREE(leasefile); + 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", @@ -3004,6 +3179,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
On 09/15/2013 11:49 PM, Nehal J Wani wrote:
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 | 177 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+)
}
+/* This function parese the leases file of dnsmasq.
s/parese/parses/
+ * + * An example of 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 * * + */ +static int +networkGetDHCPLeasesHelper(virNetworkPtr network, + virNetworkObjPtr obj, + const char *mac, + virNetworkDHCPLeasesPtr **leases) +{ + int rv = -1; + size_t i = 0; + size_t nleases = 0; + char *leasefile; + char **lease_fields; + char dhcpentry[VIR_NETWORK_DHCP_LEASE_LENGTH_MAX]; + virNetworkDHCPLeasesPtr *leases_ret = NULL; + FILE *fp = NULL; + + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, "%s", + _("no network with matching name")); + return -1; + }
Dead code (network is always non-NULL if you get here).
+ + /* Retrieve leases file location */ + leasefile = networkDnsmasqLeaseFileNameDefault(network->name); + if (!(fp = fopen(leasefile, "r"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to open leases file: %s"), leasefile); + goto cleanup; + } + + while (fgets(dhcpentry, sizeof(dhcpentry), fp) != NULL) {
Rather than hand-rolling your file reading, I would suggest using virFileReadAll(leasefile, len, &buf), and then loop over the returned buffer. That gives you nicer guarantees about handling things like interrupts that fgets() might get confused on.
+ virNetworkDHCPLeasesPtr lease = NULL; + + /* Remove newline */ + dhcpentry[strlen(dhcpentry) - 1] = '\0'; + + /* Split the lease line */ + lease_fields = virStringSplit(dhcpentry, " ", VIR_NETWORK_DHCP_LEASE_FIELDS); + + if (virStringListLength(lease_fields) != 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 && STRNEQ(mac, lease_fields[1]))
This may not do what you want - remember, 'mac' is specified by the user, whereas lease_fields[1] is specified by dnsmasq. While we can reasonably assume that dnsmasq always uses a canonical MAC representation (such as 52:54:00:1a:0a:6d), the user may have passed in a non-canonical form that we can still parse and recognize as the same address (such as 52:54:00:1A:0a:6D, or even 52:54:0:1a:a:6d). You want to use virMacAddrCompare() instead of STRNEQ.
+ continue;
Memleak. lease_fields is allocated each iteration of the loop, so it must be freed each iteration of the loop.
+ + if (VIR_EXPAND_N(leases_ret, nleases, 1) < 0) + goto error; + + if (VIR_ALLOC(leases_ret[nleases - 1]) < 0) + goto error;
Rather than trying to allocate it into the destination array, it might be better to allocate into a local variable, and only on successful population of the entire element, use VIR_INSERT_ELEMENT to move it into the array at that time.
+ + lease = leases_ret[nleases - 1]; + + /* 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; + } + + /* Hardcoded, as dnsmasq uses ipv4 */ + lease->type = VIR_IP_ADDR_TYPE_IPV4; + lease->prefix = virSocketAddrGetIpPrefix(&obj->def->ips->address, + &obj->def->ips->netmask, + obj->def->ips->prefix); + + if ((VIR_STRDUP(lease->mac, lease_fields[1]) < 0) || + (VIR_STRDUP(lease->ipaddr, lease_fields[2]) < 0) || + (VIR_STRDUP(lease->hostname, lease_fields[3]) < 0) || + (VIR_STRDUP(lease->clientid, lease_fields[4]) < 0)) + goto error; + }
Transfer semantics rather than VIR_STRDUP might save some memory pressure.
+ + if (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: + VIR_FORCE_FCLOSE(fp); + VIR_FREE(leasefile); + return rv; + +error: + if (leases_ret) { + for (i = 0; i < nleases; i++) + virNetworkDHCPLeaseFree(leases_ret[i]); + } + VIR_FREE(leases_ret); + goto cleanup; +} +
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 default Expiry Time MAC address Protocol IP address Hostname ClientId ------------------------------------------------------------------------------------------------ 13-09-2013 03:45:31 52:54:00:20:70:3d ipv4 192.168.105.240/24 f18 * 13-09-2013 03:32:31 52:54:00:b1:70:19 ipv4 192.168.105.201/24 LDAP * tools/virsh.pod * Document new command --- tools/virsh-network.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 6 +++ 2 files changed, 106 insertions(+) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 8ddd5ca..9543c64 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1129,6 +1129,100 @@ 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 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 = vshCommandOptNetworkBy(ctl, cmd, &name, + VSH_BYNAME | VSH_BYUUID))) + 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; + } + + vshPrintExtra(ctl, " %-20s %-20s %-10s %-20s %-12s %s\n%s%s\n", + _("Expiry Time"), _("MAC address"), _("Protocol"), + _("IP address"), _("Hostname"), _("ClientId"), + "------------------------------------------------", + "------------------------------------------------"); + + for (i = 0; i < nleases; i++) { + const char *type = 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), "%d-%m-%Y %H:%M:%S", &ts); + + switch (lease->type) { + case VIR_IP_ADDR_TYPE_IPV4: + type = "ipv4"; + break; + case VIR_IP_ADDR_TYPE_IPV6: + type = "ipv6"; + break; + } + + vshPrintExtra(ctl, " %-20s %-20s %-10s %s/%-5d %-12s %s\n", + expirytime, lease->mac, type, lease->ipaddr, + lease->prefix, lease->hostname, lease->clientid); + } + + 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 +1303,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 0ae5178..b87f646 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2325,6 +2325,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
On 09/15/2013 11:49 PM, Nehal J Wani wrote:
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
+ +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 = vshCommandOptNetworkBy(ctl, cmd, &name, + VSH_BYNAME | VSH_BYUUID))) + return false;
Just use vshCommandOptNetwork (no need to use the filtering of NetworkBy).
+ + 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; + } + + vshPrintExtra(ctl, " %-20s %-20s %-10s %-20s %-12s %s\n%s%s\n", + _("Expiry Time"), _("MAC address"), _("Protocol"), + _("IP address"), _("Hostname"), _("ClientId"), + "------------------------------------------------", + "------------------------------------------------"); + + for (i = 0; i < nleases; i++) {
Should you qsort() the list before printing it, to ensure that identical MAC lines are grouped even if the driver didn't hand them back in any particular order?
+ const char *type = 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), "%d-%m-%Y %H:%M:%S", &ts);
Please use ISO %Y-%m-%d, as it is sortable, and also has the benefit of being unambiguous in both US and UK.
+ + switch (lease->type) { + case VIR_IP_ADDR_TYPE_IPV4: + type = "ipv4"; + break; + case VIR_IP_ADDR_TYPE_IPV6: + type = "ipv6"; + break; + } + + vshPrintExtra(ctl, " %-20s %-20s %-10s %s/%-5d %-12s %s\n", + expirytime, lease->mac, type, lease->ipaddr,
If a third lease->type is introduced in a future libvirtd, then you risk passing NULL to printf(%s). Please use NULLSTR(type), so that you don't crash on non-glibc systems.
+ lease->prefix, lease->hostname, lease->clientid);
Do we guarantee that ALL string fields of lease are populated with non-NULL strings (even if the empty string), no matter what? Or should we revisit the docs in 1/4, and the RPC implementation of 2/4, to allow for NULL strings for fields that aren't available?
+ } + + 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 +1303,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
Not your fault, but we should be using trailing commas when doing C99 struct initialization.
+ }, {.name = NULL} }; diff --git a/tools/virsh.pod b/tools/virsh.pod index 0ae5178..b87f646 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2325,6 +2325,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>
List it as '[I<mac>]', to make it obvious the mac address is optional.
+ +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
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Nehal J Wani