[libvirt] [for 1.2.6] Redundancy of virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC

[Daniel P. Berrange]

On Thu, Jun 26, 2014 at 06:00:22PM +0530, Nehal J Wani wrote:
> On Thu, Jun 26, 2014 at 4:58 PM, Peter Krempa <pkrempa at redhat.com> wrote:
> > Hi,
> >
> > when reviewing the patch to add python bindings for the said APIs it
> > occurred to me that the two APIs are so close in their prototypes and
> > way of functioning that we could actually merge them into one.
> >
> > Both of those return a list of lease structures and the only difference
> > is the presence of the @mac argument.
> >
> > We could unify those two APIs into one with the following signature:
> >
> > int
> > virNetworkGetDHCPLeases(virNetworkPtr network,
> >                         const char *mac,
> >                         virNetworkDHCPLeasePtr **leases,
> >                         unsigned int flags)
> >
> > And tweak the semantics of @mac where when the user passes NULL we'd
> > return the complete unfiltered list.
> >
> > This would simplify our API and also the python bindings.
> >
> > If we decide this is a good idea (in time for the release) I'll post
> > patches to flesh out the redundant parts.
> >
> > Peter
> >
> 
> 
> A long long while ago, there was already a discussion on this
> 
> References:
> (i) http://www.redhat.com/archives/libvir-list/2013-July/msg01609.html
> (ii) http://www.redhat.com/archives/libvir-list/2013-July/msg01623.html
> (iii) http://www.redhat.com/archives/libvir-list/2013-July/msg01624.html
> 
> For TL;DR:
> 
> Message 1:
> 
> At a conceptual level, what you're after here is a list of all the IP,
> mac address mappings of the virtual network. This information is useful
> even outside the context of the hypervisor driver method you're working
> on. So we should create formal APIs for exposing this, something like:
> 
>    virNetworkGetDHCPLeases(virNetworkPtr network,
>                            virNetworkDHCPLeasePtr *leases,
>                            unsigned int nleases);
> 
> And/or this
> 
>    virNetworkGetDHCPLeaseForMAC(virNetworkPtr network,
>                                 unsigned char *macaddr,
>                                 virNetworkDHCPLeasePtr lease);
> 
> and a corresponding  'virsh net-dhcp-leases <netname>' command

Unfortunately I didn't realize at the time, but my idea here
was retarded. The reason I suggested having separate APIs is
because it would make the 'ForMAC' case more app friendly as
they'd only need to pass in a existing virNetworkDHCPLeasePtr
instance, and not have to deal with dynamically allocated
lists of leases.

Of course what I completely missed was that even in the ForMAC
case, we have to return a dynamic list of leases, because you
can have both IPv4 and IPv6 leases for the same MAC. This
basically kills the main compelling reason to have 2 separate
APIs.

So in retrospect I was wrong, and I agree with Peter that we
should kill the ForMAC API and just add an (optional) macaddr
parameter to the first API. Of course we can only decided to
do this now before we release.

Other opinions...

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|