Re: [libvirt] [PATCHv3 1/4] net-dhcp-leases: Implement the public APIs
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
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)