[libvirt] [PATCHv3 1/4] net-dhcp-leases: Implement the public APIs

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130916/a524fd93/attachment-0001.sig>