On 09/26/2013 02:08 AM, Nehal J Wani wrote:
> Introduce 3 new APIs, virNetworkGetDHCPLeases, virNetworkGetDHCPLeasesForMAC
> and virNetworkDHCPLeaseFree.
>
> * virNetworkGetDHCPLeases: returns the dhcp leases information for a given
> virtual network.
>
> For DHCPv4, the information includes:
> - Expirytime
> - MAC Address
> - IPv4 address (with type and prefix)
> - Hostname (can be NULL)
> - Client ID (can be NULL)
>
> For DHCPv6, the information includes
> - Expirytime
> - IAID
> - IPv6 address (with type and prefix)
> - Hostname (can be NULL)
> - Client DUID
>
> * 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.
> +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases;
> +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr;
> +struct _virNetworkDHCPLeases {
> + long long expirytime; /* Seconds since epoch */
> + union {
> + char *mac; /* MAC address */
> + unsigned long iaid; /* Identity association identifier (IAID) */
> + } id;
I'm not sure I like iaid - the whole point of this interface was to
return IP addresses associated with a MAC. Either the iaid is important
and deserves a separate field, or all we care about is the MAC address.
Not to mention that you didn't document which leg of the id union is
valid based on the type discriminator.
> + char *ipaddr; /* IP address */
> + char *hostname; /* Hostname */
Document that this field can be NULL.
> + char *clientid; /* Client ID or DUID */
Likewise.
> + int type; /* virIPAddrType */
If you DO keep the union (which I'm not too sure about), then convention
is to put the discriminator first, not separated by other fields.
> + unsigned int prefix; /* IP address prefix */
This field may be nicer if placed next to ipaddr, since it is more
closely related to that field.
I sense some struggle in getting the interface right - for everyone's
sake, let's FIRST get the interface nailed down, rather than churning on
implementations of something that then has to be reworked because the
interface wasn't correct.
> +
> +/**
> + * virNetworkGetDHCPLeases:
> + * @network: Pointer to network object
> + * @leases: Pointer to a variable to store the array containing details on
> + * obtained leases, or NULL if the list is not required (just returns
> + * number of leases).
> + * @flags: Extra flags, not used yet, so callers should always pass 0
> + *
> + * The API fetches lease info for all network interfaces connected to the given
> + * virtual network. In case of DHCPv4, the fields returned are expiry time, MAC
> + * address, IPv4 address, hostname and client ID, out of which, hostname and
> + * client ID can be NULL. In case of DHCPv6, the fields returned are expiry time,
> + * IAID, IPv6 address, hostname and client DUID, out of which, only hostname can
> + * be NULL.
> + *
> + * 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);
This example does not match the struct above (at least lease-mac would
have to be converted to lease->id.mac, if you keep the union). Make
sure that the example you list in the documentation comments would
actually compile, if someone copies it into their code.
Might be worth adding:
See also virNetworkGetDHCPLeasesForMAC() as a convenience for filtering
the list to a single MAC address.
> +
> + if (conn->networkDriver && conn->networkDriver->networkGetDHCPLeases) {
> + int ret;
> + ret = conn->networkDriver->networkGetDHCPLeases(network,leases, flags);
Space after comma.
> +/**
> + * virNetworkGetDHCPLeasesForMAC:
> + * @network: Pointer to network object
> + * @mac: ASCII formatted MAC address of an interface
> + * @leases: Pointer to a variable to store the array containing details on
> + * obtained leases, or NULL if the list is not required (just returns
> + * number of leases).
> + * @flags: extra flags, not used yet, so callers should always pass 0
> + *
> + * The API fetches leases info of the interface which matches with the
> + * given @mac. There can be multiple leases for a single @mac because this
> + * API supports 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.
Might be worth adding:
See virNetworkGetDHCPLeases() for more details on list contents and
proper cleanup.
--
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