On Wed, Aug 14, 2013 at 04:57:51AM +0530, Nehal J. Wani wrote: > On Wed, Aug 14, 2013 at 4:29 AM, Eric Blake <eblake@xxxxxxxxxx> 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); In common with my feedback on the other IP addr API, I'd suggest that we should use virNetworkDHCPLeasesPtr **leases, e an array of pointers to objects, not an array of objects. And remove the nleases parameter - just use the return value Also I'd like to see two separate APIs here - one to get a list of all leases, and one to get the single lease for a specific MAC address. 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list