[libvirt] RFC: Introduce API to return configuration/state paths of the network driver
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: diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a7ff602..7274861 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -124,7 +124,7 @@ static int networkUnplugBandwidth(virNetworkObjPtr net, static struct network_driver *driverState = NULL; -static char * +char * networkDnsmasqLeaseFileNameDefault(const char *netname) { char *leasefile; diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 50258b5..40e3990 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -49,6 +49,8 @@ int networkDnsmasqConfContents(virNetworkObjPtr network, char **configstr, dnsmasqContext *dctx, dnsmasqCapsPtr caps); +char * networkDnsmasqLeaseFileNameDefault(const char *netname) + ATTRIBUTE_NONNULL(1); # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(iface) 0 @@ -57,6 +59,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network, # define networkGetNetworkAddress(netname, netaddr) (-2) # define networkDnsmasqConfContents(network, pidfile, configstr, \ dctx, caps) 0 +# define networkDnsmasqLeaseFileNameDefault(netname) 0 # endif typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); Similar hack has been used so that networkAllocateActualDevice() can be called from qemu_command.c. Although the above method works, we want to have a formal API and not leave things like a hack. /* * @conn: connection object * @params: array to populate on output * @nparams: number of parameters that will be filled * @flags: not supported, user should pass 0 for now * return 0 on success -1 otherwise * Valid parameter field names: * VIR_NETWORK_CONFIG_DIR, VIR_NETWORK_AUTOSTART_DIR, VIR_STATE_DIR, * VIR_PID_DIR, VIR_DNSMASQ_STATE_DIR, VIR_RADVD_STATE_DIR * All the above will of the type VIR_TYPED_PARAM_STRING */ int virNetworkGetConfigFileName(virConnectPtr conn, virTypedParameterPtr params, int nparams, unsigned int flags) Nehal J. Wani UG3, BTech CS+MS(CL) IIIT-Hyderabad http://commandlinewani.blogspot.com
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 -- |: 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 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.
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. 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 25/07/13 18:53, 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
On 25/07/13 17:35, Daniel P. Berrange wrote: 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
On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: 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. osier
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 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
On 25/07/13 17:35, Daniel P. Berrange wrote: 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
On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: 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. 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 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 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
On 25/07/13 17:35, Daniel P. Berrange wrote: 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
On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: 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. osier
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 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
On 25/07/13 17:35, Daniel P. Berrange wrote: 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
On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: 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. osier
On Thu, Jul 25, 2013 at 07:37:22PM +0800, Osier Yang wrote:
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 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
On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote: 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.
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 -- |: 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 07/25/2013 03:35 AM, Daniel P. Berrange wrote:
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);
What struct contents do you propose for virNetworkDHCPLeasePtr? Are we expressing the output in stringized or raw form? That is, would a MAC address represented in that struct take exactly 6 bytes, even if some of them are the NUL byte, or would it be the stringized 18-byte NUL-terminated string?
And/or this
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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
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@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);
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 :|
On 15/08/13 19:32, Daniel P. Berrange wrote:
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@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); 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 Agreed.
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.
I'm wondering what's the principle of introducing a new API now, previously in cases similar with this, we would like have a single API instead of two, I.E. make the API more general enough if the intended purposes could be aggregated into one. Did we see the simple enough APIs are more popular for upper apps, and change to have more simple API instead? Osier
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Nehal J. Wani -
Osier Yang