[libvirt] new API to get list of *all* interfaces matching a MAC

(This probably seems like overanalysis of a simple problem. That's what I'm best at ;-) Due to oversight, the function virInterfaceLookupByMACString() can only return a single interface, but it's possible that a host may have more than one interface with the same MAC. Since the API had already been released by the time we realized this, the existing function will remain and a new one added that can return a list of interfaces. This new API will need to deal with the fact that the list length is effectively unbounded. I can see three ways of dealing with this, and want to learn which is preferred by others before spending time on the implementation. 1) The array containing the list is allocated by libvirt, freed by caller. int virInterfaceLookupAllByMACString(virConnectPtr conn, const char *mac, virInterfacePtr **matches); "matches" will point to an array of virInterfacePtr, and that array's length will be the return value. This array will be allocated by libvirt, but must be freed by the application when it's finished. Usage (ignoring errors, of course :-): virInterfacePtr *matches; int matchCt; matchCt = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", &matches); for (i = 0; i < matchCt; i++) { /* do something with an interface */ virInterfaceFree(matches[i]); } free(matches); 2) The array containing the list is allocated by the application, and its length sent to libvirt. Libvirt then returns the actual number of matches, which may be greater than the max length sent by the application, but only copies as many as the application asked for. If the application sends 0 as maxMatches and/or a NULL pointer instead of a pointer to an array, libvirt will return the actual length, but not the list itself. For example, if there are 3 interfaces matching, and maxMatches is set to 2, the function will still return 3, but only set two items in the matches array. int virInterfaceLookupAllByMACString(virConnectPtr conn, const char *mac, virInterfacePtr **matches, int maxMatches); This puts all the functionality needed in a single API function, but it will generally need to be called twice - once to learn the length, and the second (after allocating an array of appropriate size) to retrieve the list. virInterfacePtr *matches; int matchCtA, matchCtB; matchCtA = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", NULL, 0); matches = malloc (matchCt * sizeof(virInterfacePtr)); matchCtB = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", &matches, matchCt); for (i = 0; i < min(matchCtA, matchCtB); i++) { /* do something with an interface */ virInterfaceFree(matches[i]); } free(matches); 3) Again, the array containing the list is allocated by the application, but it learns the proper length to allocate by calling a different API function. The lookup function itself returns the number of interfaces delivered in the array, rather than the number of matches it *wanted* to put in the array. int virInterfaceCountAllByMACString(virConnectPtr conn, const char *mac); int virInterfaceLookupAllByMACString(virConnectPtr conn, const char *mac, virInterfacePtr **matches, int maxMatches); Usage: virInterfacePtr *matches; int matchCt; matchCt = virInterfaceCountAllByMACString(conn, "00:01:02:03:04:05"); matches = malloc (matchCt * sizeof(virInterfacePtr)); matchCt = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", &matches, matchCt); for (i = 0; i < matchCt; i++) { /* do something with an interface */ virInterfaceFree(matches[i]); } free(matches); ******* (3) is closest to behavior of other libvirt APIs, but requires more functions in the API, and could lead to situations where newly added interfaces aren't in the list (if a new interface is added between calling the Count function and the Lookup function) . (2) has a more compact API (one which matches the netcf API exactly), but has the same problems with potential bad counts. (1) seems like the cleanest API, but I don't know what others' opinions are on having libvirt allocate the array (since that's not how its done for other similar things, eg virConnectListInterface, virConnectListNetworks, etc), or maybe there's some limitation of the RPC I haven't considered that makes it unfeasible. Which should I implement?

On Mon, Aug 24, 2009 at 03:24:13PM -0400, Laine Stump wrote:
(This probably seems like overanalysis of a simple problem. That's what I'm best at ;-)
Due to oversight, the function virInterfaceLookupByMACString() can only return a single interface, but it's possible that a host may have more than one interface with the same MAC. Since the API had already been released by the time we realized this, the existing function will remain and a new one added that can return a list of interfaces. This new API will need to deal with the fact that the list length is effectively unbounded. I can see three ways of dealing with this, and want to learn which is preferred by others before spending time on the implementation.
1) The array containing the list is allocated by libvirt, freed by caller.
int virInterfaceLookupAllByMACString(virConnectPtr conn, const char *mac, virInterfacePtr **matches);
"matches" will point to an array of virInterfacePtr, and that array's length will be the return value. This array will be allocated by libvirt, but must be freed by the application when it's finished.
Usage (ignoring errors, of course :-):
virInterfacePtr *matches; int matchCt;
matchCt = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", &matches); for (i = 0; i < matchCt; i++) { /* do something with an interface */ virInterfaceFree(matches[i]); } free(matches);
[...]
(3) is closest to behavior of other libvirt APIs, but requires more functions in the API, and could lead to situations where newly added interfaces aren't in the list (if a new interface is added between calling the Count function and the Lookup function) . (2) has a more compact API (one which matches the netcf API exactly), but has the same problems with potential bad counts. (1) seems like the cleanest API, but I don't know what others' opinions are on having libvirt allocate the array (since that's not how its done for other similar things, eg virConnectListInterface, virConnectListNetworks, etc), or maybe there's some limitation of the RPC I haven't considered that makes it unfeasible.
I would tend to prefer 1) at this point too, I don't see why 1) whould be harder than 2 or 3 for RPCs or something, and in any case the python bindings will have to be handcoded for any of the 3. IMHO 2 and 3 both suffer a race condition where you call to get the length and when you actually make the request another interface may have been added in the meantime, even if interface activation is slow, we should try to avoid this now IMHO. The thing which is the hardest IMHO is properly describing and try to make sure all client apps using any of the 3 apis will correctly call virInterfaceFree() on each of the returned item of the array. Maybe having the example in the function comment could help, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Mon, Aug 24, 2009 at 03:24:13PM -0400, Laine Stump wrote:
(This probably seems like overanalysis of a simple problem. That's what I'm best at ;-)
Due to oversight, the function virInterfaceLookupByMACString() can only return a single interface, but it's possible that a host may have more than one interface with the same MAC. Since the API had already been released by the time we realized this, the existing function will remain and a new one added that can return a list of interfaces. This new API will need to deal with the fact that the list length is effectively unbounded. I can see three ways of dealing with this, and want to learn which is preferred by others before spending time on the implementation.
I'm rather wondering why exactly we need another API. For APIs which return a singleton result we have LookupByXXX methods, since it gives apps a good efficiency win making the API O(1), instead of O(n). We already have an API for listing all interfaces. An application can simply use that and then discard the results which have a non-matching MAC address. THis is O(n) with the existing API we have for listing interfaces, and would be O(n) with any new API we might add. So I don't really see any point in adding more to list filtering on MAC address. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Tue, Aug 25, 2009 at 10:15:05AM +0100, Daniel P. Berrange wrote:
On Mon, Aug 24, 2009 at 03:24:13PM -0400, Laine Stump wrote:
(This probably seems like overanalysis of a simple problem. That's what I'm best at ;-)
Due to oversight, the function virInterfaceLookupByMACString() can only return a single interface, but it's possible that a host may have more than one interface with the same MAC. Since the API had already been released by the time we realized this, the existing function will remain and a new one added that can return a list of interfaces. This new API will need to deal with the fact that the list length is effectively unbounded. I can see three ways of dealing with this, and want to learn which is preferred by others before spending time on the implementation.
I'm rather wondering why exactly we need another API.
Basically to be able to obsolete the broken one, and explain in the documentation the function limitation and suggest using the new one. Way simpler to explain than suggesting doing the full extract and leave the user filter. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Tue, Aug 25, 2009 at 04:04:44PM +0200, Daniel Veillard wrote:
On Tue, Aug 25, 2009 at 10:15:05AM +0100, Daniel P. Berrange wrote:
On Mon, Aug 24, 2009 at 03:24:13PM -0400, Laine Stump wrote:
(This probably seems like overanalysis of a simple problem. That's what I'm best at ;-)
Due to oversight, the function virInterfaceLookupByMACString() can only return a single interface, but it's possible that a host may have more than one interface with the same MAC. Since the API had already been released by the time we realized this, the existing function will remain and a new one added that can return a list of interfaces. This new API will need to deal with the fact that the list length is effectively unbounded. I can see three ways of dealing with this, and want to learn which is preferred by others before spending time on the implementation.
I'm rather wondering why exactly we need another API.
Basically to be able to obsolete the broken one, and explain in the documentation the function limitation and suggest using the new one. Way simpler to explain than suggesting doing the full extract and leave the user filter.
I don't buy that argument. We have a clear split between vir*Lookup* methods returning a single object, and vir*List* methods giving back multiple objects, which is consistent across all the APIs. I don't think we need to do a specialcase for network interfaces, for something that'll rarely be an issue in the real world. The existing behaviour is easy to understand & document & deal with from applicaton code as it is. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Tue, Aug 25, 2009 at 03:10:10PM +0100, Daniel P. Berrange wrote:
On Tue, Aug 25, 2009 at 04:04:44PM +0200, Daniel Veillard wrote:
On Tue, Aug 25, 2009 at 10:15:05AM +0100, Daniel P. Berrange wrote:
On Mon, Aug 24, 2009 at 03:24:13PM -0400, Laine Stump wrote:
(This probably seems like overanalysis of a simple problem. That's what I'm best at ;-)
Due to oversight, the function virInterfaceLookupByMACString() can only return a single interface, but it's possible that a host may have more than one interface with the same MAC. Since the API had already been released by the time we realized this, the existing function will remain and a new one added that can return a list of interfaces. This new API will need to deal with the fact that the list length is effectively unbounded. I can see three ways of dealing with this, and want to learn which is preferred by others before spending time on the implementation.
I'm rather wondering why exactly we need another API.
Basically to be able to obsolete the broken one, and explain in the documentation the function limitation and suggest using the new one. Way simpler to explain than suggesting doing the full extract and leave the user filter.
I don't buy that argument. We have a clear split between vir*Lookup* methods returning a single object, and vir*List* methods giving back multiple objects, which is consistent across all the APIs. I don't think we need to do a specialcase for network interfaces, for something that'll rarely be an issue in the real world. The existing behaviour is easy to understand & document & deal with from applicaton code as it is.
And I don't buy that argument of homogenety of interface because precisely the interfaces while named similary have actually different behaviour in the case of Interface. And it's way easier to deal with suggesting to use the right API directly, than wait for user error which as you said will occur infrequently but will require a change at the application level. If virDomainLookupByName could miss some domains, I would definitely try to fix with a proper API to replace. I think the fact it's about Interface doesn't make that less the right thing to do. The goal of the API is making sure the apps will be fine, not making sure it's simpler for the app developper. That's the reason why apis like gethostbyname and similar gets deprecated. Interface based code in apps is yet to be written, let's make sure it's written correctly. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange
-
Daniel Veillard
-
Laine Stump