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

On Wed, Oct 02, 2013 at 03:41:56PM +0530, Nehal J Wani wrote: > On Wed, Oct 2, 2013 at 3:18 PM, Daniel P. Berrange <berrange(a)redhat.com&gt; wrote: > > On Wed, Oct 02, 2013 at 03:08:00PM +0530, Nehal J Wani wrote: > >> On Wed, Oct 2, 2013 at 1:43 PM, Daniel P. Berrange <berrange(a)redhat.com&gt; wrote: > >> > On Tue, Oct 01, 2013 at 05:39:02PM -0600, Eric Blake wrote: > >> >> 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. > >> > > >> > Agreed, we want the MAC address to be unconditionally available > >> > here. IMHO the IAID is not something we care about exposing. That > >> > is a impl detail of the DHCP comms protocol that is not useful > >> > to people outside. > >> > > >> > >> So in case DHCPv6 is used by the client, should we report the rest of > >> the lease fields and report MAC as NULL? > > > > No, we must report the MAC. This data is useless without the MAC address > > being present. > > > > You can't even implement the virNetworkGetDHCPLeasesForMAC API without > > knowing the MAC for a lease. > > The issue is, in case of leases containing ipv6 addresses, there is no > field for MAC address. Laine suggested extracting MAC address from the > cliend DUID. For example: > > 1380692760 52:54:00:e7:85:1e 192.168.122.116 * * > duid 00:01:00:01:19:dd:fb:37:f0:4d:a2:8c:14:51 > 1380692762 15172894 2001:db8:ca2:2:1::de * > 00:01:00:01:19:dd:fb:af:52:54:00:e7:85:1e We don't want to design the API around the limitations of one particular DHCP server implementation. If dnsmasq's leases file can't give us the MAC addr, we still want to allow for MAC in the public struct. Have you asked the dnsmasq developers if they're willing to add a field for the MAC addr to the leases file for IPv6 ?