On Wed, Aug 14, 2013 at 4:29 AM, Eric Blake <eblake@redhat.com> wrote:
On 08/13/2013 04:48 PM, Eric Blake wrote:
virNetworkGetDHCPLeaseForMAC(virNetworkPtr network, unsigned char *macaddr,
I personally think the public API should stick to stringized representations. Yes, it's less friendly to machine code, but internally, our src/util/virmacaddr.h helps transfer between stringized and binary. Furthermore, we already have other API that operates on stringized versions, but none that operates on raw (see virDomainSetInterfaceParameters). Knowing what representation we are targetting impacts how this API will look.
For comparison, look at the API for virDomainLookupByUUID (takes a raw uuid - fixed number of bytes, unsigned char* argument) vs. virDomainLookupByUUIDString (takes a stringized uuid, char* argument). If efficiency were a concern, then I'd propose that we have two API:
virNetworkGetDHCPLeaseForMAC(virNetworkPtr network, unsigned char *macaddr, ...)
virNetworkGetDHCPLeaseForMACString(virNetworkPtr network, char *macaddr, ...)
But I don't think efficiency is a concern, and rather than add a new API that has to have dual forms, I'd rather stick with the shorter name but using the stringized form of MAC.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
I would like to see the API as: /** * virNetworkGetDHCPLeases: * @network: pointer to network object * @macaddr: MAC Address of an interface * @leases: pointer to an array of structs which will hold the leases * @nleases: number of leases in output * @flags: extra flags, not used yet, so callers should always pass 0 * * The API returns the leases of all interfaces by default, and if * @macaddr is specified,.only the lease of the interface which * matches the @macaddr is returned. * * Returns number of leases on success, -1 otherwise */ int virNetworkGetDHCPLeases(virNetworkPtr network, const char *macaddr, virNetworkDHCPLeasesPtr *leases, int *nleases, unsigned int flags); Structs to be used: /*In include/libvirt/libvirt.h.in */ struct _virNetworkDHCPLeases { long long expirytime; char *macaddr; char *ipaddr; char *hostname; char *clientid; }; /*In src/remote/remote_protocol.x*/ struct remote_network_dhcp_leases { hyper expirytime; remote_nonnull_string macaddr; remote_nonnull_string ipaddr; remote_nonnull_string hostname; remote_nonnull_string clientid; }; struct remote_network_get_dhcp_leases_args { remote_nonnull_network net; remote_string macaddr; unsigned int flags; }; struct remote_network_get_dhcp_leases_ret { remote_network_dhcp_leases leases<>; }; The following two blocks are required more than one, so should we be introducing helper functions for them? if ((VIR_STRDUP((*leases)[0].macaddr, leaseparams[1]) < 0) || (VIR_STRDUP((*leases)[0].ipaddr, leaseparams[2]) < 0) || (VIR_STRDUP((*leases)[0].hostname, leaseparams[3]) < 0) || (VIR_STRDUP((*leases)[0].clientid, leaseparams[4]) < 0)) goto cleanup; for (i = 0; i < nleases; i++) { VIR_FREE(leases[i].macaddr); VIR_FREE(leases[i].ipaddr); VIR_FREE(leases[i].hostname); VIR_FREE(leases[i].clientid); } -- Nehal J. Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad http://commandlinewani.blogspot.com