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