On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote: > Currently, there is no API which returns configuration/state paths of the > network driver. > Although it is a private implementation of the network driver, I don't see > any harm in > making the locations public because although the locations might change, > there will always > be a location for these files. There is a need for this API to implement > method 2 of the > "API to query ip addresses of a given domain", refer: > http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is > required to parse > the leases file generated by dnsmasq. So, this API will be used by the qemu > driver, but it > can also be made public, so that, if a user wants to know get some > information from a > configuration file, he can get the location from libvirt and analyze it on > his own. Right now, > there is an alternate way to get the info: by using > networkDnsmasqLeaseFileNameDefault, > defined in /src/network/bridge_driver.c Since this function is static, it > is part of the private > implementation and not visible outside. To make it public, the following > hack is possible: NACK, As I explained on IRC, the hypervisor drivers have no business accessing the dnsmasq lease files from the bridge driver. This is considered to be a private implementation detail. 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

On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: > On 25/07/13 17:35, Daniel P. Berrange wrote: >> On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote: >>> Currently, there is no API which returns configuration/state paths of the >>> network driver. >>> Although it is a private implementation of the network driver, I don't see >>> any harm in >>> making the locations public because although the locations might change, >>> there will always >>> be a location for these files. There is a need for this API to implement >>> method 2 of the >>> "API to query ip addresses of a given domain", refer: >>> http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is >>> required to parse >>> the leases file generated by dnsmasq. So, this API will be used by the qemu >>> driver, but it >>> can also be made public, so that, if a user wants to know get some >>> information from a >>> configuration file, he can get the location from libvirt and analyze it on >>> his own. Right now, >>> there is an alternate way to get the info: by using >>> networkDnsmasqLeaseFileNameDefault, >>> defined in /src/network/bridge_driver.c Since this function is static, it >>> is part of the private >>> implementation and not visible outside. To make it public, the following >>> hack is possible: >> NACK, >> >> As I explained on IRC, the hypervisor drivers have no business accessing >> the dnsmasq lease files from the bridge driver. This is considered to be >> a private implementation detail. >> >> 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); > i'm wondering if it should be more than just the lease file path, e.g. > also the $net.conf, $net-radvd.conf, etc, though they are useless > now, but may be useful in future, i.e. to have a more general api > than this one. and in that case, it should return an array of typed > parameter instead. We've already discussed this in the context of the virDomain API for getting IP addresses & decided that virTypedParameter was not appropriate there & we'd use a struct. The same arguments apply here IMHO.

On Thu, Jul 25, 2013 at 07:01:05PM +0800, Osier Yang wrote: > On 25/07/13 18:53, Daniel P. Berrange wrote: >> On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: >>> On 25/07/13 17:35, Daniel P. Berrange wrote: >>>> On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote: >>>>> Currently, there is no API which returns configuration/state paths of the >>>>> network driver. >>>>> Although it is a private implementation of the network driver, I don't see >>>>> any harm in >>>>> making the locations public because although the locations might change, >>>>> there will always >>>>> be a location for these files. There is a need for this API to implement >>>>> method 2 of the >>>>> "API to query ip addresses of a given domain", refer: >>>>> http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is >>>>> required to parse >>>>> the leases file generated by dnsmasq. So, this API will be used by the qemu >>>>> driver, but it >>>>> can also be made public, so that, if a user wants to know get some >>>>> information from a >>>>> configuration file, he can get the location from libvirt and analyze it on >>>>> his own. Right now, >>>>> there is an alternate way to get the info: by using >>>>> networkDnsmasqLeaseFileNameDefault, >>>>> defined in /src/network/bridge_driver.c Since this function is static, it >>>>> is part of the private >>>>> implementation and not visible outside. To make it public, the following >>>>> hack is possible: >>>> NACK, >>>> >>>> As I explained on IRC, the hypervisor drivers have no business accessing >>>> the dnsmasq lease files from the bridge driver. This is considered to be >>>> a private implementation detail. >>>> >>>> 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); >>> i'm wondering if it should be more than just the lease file path, e.g. >>> also the $net.conf, $net-radvd.conf, etc, though they are useless >>> now, but may be useful in future, i.e. to have a more general api >>> than this one. and in that case, it should return an array of typed >>> parameter instead. >> We've already discussed this in the context of the virDomain API for >> getting IP addresses & decided that virTypedParameter was not appropriate >> there & we'd use a struct. The same arguments apply here IMHO. >> > the api to get the ip addresses is more complicate than this, and we > finally chose the struct is because of the multiple level information > is hard to constuct with typed parameter, but for this api, it's different, > as it just needs to return the file paths. No, file paths will absolutely never be exposed outside of the bridge driver. The API I suggest above are about exposing the IP address + MAC address of current leases. ie the actual data the user needs, *not* the file path containing the data which is a private impl detail.

On 25/07/13 19:21, Osier Yang wrote: >On 25/07/13 19:13, Daniel P. Berrange wrote: >>On Thu, Jul 25, 2013 at 07:01:05PM +0800, Osier Yang wrote: >>>On 25/07/13 18:53, Daniel P. Berrange wrote: >>>>On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: >>>>>On 25/07/13 17:35, Daniel P. Berrange wrote: >>>>>>On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote: >>>>>>>Currently, there is no API which returns >>>>>>>configuration/state paths of the >>>>>>>network driver. >>>>>>>Although it is a private implementation of the network >>>>>>>driver, I don't see >>>>>>>any harm in >>>>>>>making the locations public because although the >>>>>>>locations might change, >>>>>>>there will always >>>>>>>be a location for these files. There is a need for >>>>>>>this API to implement >>>>>>>method 2 of the >>>>>>>"API to query ip addresses of a given domain", refer: >>>>>>>http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html >>>>>>>. It is >>>>>>>required to parse >>>>>>>the leases file generated by dnsmasq. So, this API >>>>>>>will be used by the qemu >>>>>>>driver, but it >>>>>>>can also be made public, so that, if a user wants to know get some >>>>>>>information from a >>>>>>>configuration file, he can get the location from >>>>>>>libvirt and analyze it on >>>>>>>his own. Right now, >>>>>>>there is an alternate way to get the info: by using >>>>>>>networkDnsmasqLeaseFileNameDefault, >>>>>>>defined in /src/network/bridge_driver.c Since this >>>>>>>function is static, it >>>>>>>is part of the private >>>>>>>implementation and not visible outside. To make it >>>>>>>public, the following >>>>>>>hack is possible: >>>>>>NACK, >>>>>> >>>>>>As I explained on IRC, the hypervisor drivers have no >>>>>>business accessing >>>>>>the dnsmasq lease files from the bridge driver. This is >>>>>>considered to be >>>>>>a private implementation detail. >>>>>> >>>>>>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); >>>>>i'm wondering if it should be more than just the lease >>>>>file path, e.g. >>>>>also the $net.conf, $net-radvd.conf, etc, though they are useless >>>>>now, but may be useful in future, i.e. to have a more general api >>>>>than this one. and in that case, it should return an array of typed >>>>>parameter instead. >>>>We've already discussed this in the context of the virDomain API for >>>>getting IP addresses & decided that virTypedParameter was >>>>not appropriate >>>>there & we'd use a struct. The same arguments apply here IMHO. >>>> >>>the api to get the ip addresses is more complicate than this, and we >>>finally chose the struct is because of the multiple level information >>>is hard to constuct with typed parameter, but for this api, >>>it's different, >>>as it just needs to return the file paths. >>No, file paths will absolutely never be exposed outside of the bridge >>driver. The API I suggest above are about exposing the IP address + MAC >>address of current leases. ie the actual data the user needs, *not* the >>file path containing the data which is a private impl detail. >> >oh, i see, agreed with the idea then. > 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.

> 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.

On Wed, Aug 14, 2013 at 4:29 AM, Eric Blake <eblake(a)redhat.com&gt; 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);