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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list