[libvirt] [for 1.2.6] Redundancy of virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC
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
On Thu, Jun 26, 2014 at 4:58 PM, Peter Krempa <pkrempa@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 Daniel Message 2: for the api interface: int virNetworkGetDHCPLeases(virNetworkPtr network, unsigned char *macaddr, virNetworkDHCPLeasePtr *leases, unsigned int nleases); i think this is better. which returns all of the leases if no mac is specified. otherwise just returns the lease of the network matches the mac. osier Message 3: I rather prefer to see separate APIs for this job as I described. Sure you could have an optional macaddr parameter, but I think it is nicer to just have clear APIs for the "list many" vs "get one" tasks. Regards, Daniel Regards, Nehal J Wani
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@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 :|
On 06/26/2014 07:36 AM, Daniel P. Berrange wrote:
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.
I'm in favor of this simplification as well.
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.
I remember the earlier debate, and think I kind of missed that point at the time, as well.
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.
Yes, now is the time to make the fix, before RC-2 is spun (so we still have some test time), and before it is baked into the 1.2.6 release. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Nehal J Wani -
Peter Krempa