On 05/24/2016 11:42 PM, Jim Fehlig wrote:
On 05/22/2016 09:34 PM, Chun Yan Liu wrote:
On 5/17/2016 at 11:46 PM, in message <2fa0cb6f-ea83-d6f3-18f8-51a671574205@laine.org>, Laine Stump <laine@laine.org> wrote: On 05/16/2016 06:05 PM, Jim Fehlig wrote: Chun Yan Liu wrote:
>> On 5/14/2016 at 07:47 AM, in message <5736677D.8030209@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: On 05/13/2016 12:21 AM, Chunyan Liu wrote: > Add .domainInterfaceAddresses so that user can have a way to > get domain interface address by 'virsh domifaddr'. Currently > it only supports '--source lease'. > > Signed-off: Chunyan Liu <cyliu@suse.com> > --- > src/libxl/libxl_driver.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 140 insertions(+) > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 062d6f8..f2bd6fa 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -5425,6 +5425,145 @@ static int libxlNodeGetSecurityModel(virConnectPtr conn, > return 0; > } > > +static int > +libxlGetDHCPInterfaces(virDomainPtr dom, > + virDomainObjPtr vm, > + virDomainInterfacePtr **ifaces) > +{ > + int rv = -1; > + int n_leases = 0; > + size_t i, j; > + size_t ifaces_count = 0; > + virNetworkPtr network = NULL; > + char macaddr[VIR_MAC_STRING_BUFLEN]; > + virDomainInterfacePtr iface = NULL; > + virNetworkDHCPLeasePtr *leases = NULL; > + virDomainInterfacePtr *ifaces_ret = NULL; > + > + if (!dom->conn->networkDriver || > + !dom->conn->networkDriver->networkGetDHCPLeases) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Network driver does not support DHCP lease query")); > + return -1; > + } > + > + for (i = 0; i < vm->def->nnets; i++) { > + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) > + continue; > + > + virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); > + virObjectUnref(network); > + network = virNetworkLookupByName(dom->conn, > + vm->def->nets[i]->data.network.name); > + > + if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, > + &leases, 0)) < 0) > + goto error; > + > + if (n_leases) { > + if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0) > + goto error; > + > + if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0) > + goto error; > + > + iface = ifaces_ret[ifaces_count - 1]; > + /* Assuming each lease corresponds to a separate IP */ > + iface->naddrs = n_leases; > + > + if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0) > + goto error; > + > + if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0) > + goto cleanup; > + > + if (VIR_STRDUP(iface->hwaddr, macaddr) < 0) > + goto cleanup; > + } > + > + for (j = 0; j < n_leases; j++) { > + virNetworkDHCPLeasePtr lease = leases[j]; > + virDomainIPAddressPtr ip_addr = &iface->addrs[j]; > + > + if (VIR_STRDUP(ip_addr->addr, lease->ipaddr) < 0) > + goto cleanup; > + > + ip_addr->type = lease->type; > + ip_addr->prefix = lease->prefix; > + } > + > + for (j = 0; j < n_leases; j++) > + virNetworkDHCPLeaseFree(leases[j]); > + > + VIR_FREE(leases); > + } > + > + *ifaces = ifaces_ret; > + ifaces_ret = NULL; > + rv = ifaces_count; > + > + cleanup: > + virObjectUnref(network); > + if (leases) { > + for (i = 0; i < n_leases; i++) > + virNetworkDHCPLeaseFree(leases[i]); > + } > + VIR_FREE(leases); > + > + return rv; > + > + error: > + if (ifaces_ret) { > + for (i = 0; i < ifaces_count; i++) > + virDomainInterfaceFree(ifaces_ret[i]); > + } > + VIR_FREE(ifaces_ret); > + > + goto cleanup; > +}
It's unfortunate this is a copy-paste from the qemu driver. The code is not trivial and any bug fixes in one copy could be missed in the other. A lot of the function is domain related, so probably can't be abstracted further to the network code. Have you considered approaches that allow the drivers to share this code? Well, it can be extracted and placed in bridge_driver.c as networkGetDHCPInterfaces, but I don't know if that is acceptable? Hmm, maybe something like networkGetDHCPLeasesAll() is a better name. Regardless of the name, I see that other functions in bridge_driver.c take a virDomainDefPtr, so maybe extracting the code to bridge_driver.c is acceptable.
Well, I don't really *like* the way that those network*() functions are implemented (just by exporting a symbol, implying that any hypervisor driver that uses them must directly link the bridge driver in, rather than being able to load an alternative), but that was the most expedient way of handling the need at the time, and nobody complained about it, so... :-)
Definitely duplication of code is bad (I say that although I do it a lot myself!). And if a function is all about "doing something with a network device / connection" and it will need access to the network object, I think the network driver is the place to have it. *AND* if it's something that's not needed directly in the public API, then making it available as a public API call is a bad idea (since you're then stuck with it forever).
However, I don't know that I like the idea of a function in the network driver that is trawling through the virDomainDef object. It may seem like a fine distinction - returning the lease info for a single interface vs. returning the lease info for all interfaces in the domain, but it does take the co-mingling between the network and hypervisor drivers to a new level. Yes, it's true that there are already functions that are part of this "backend super double secret network API" (watch Animal House and you'll understand the reference) that take a virDomainDefPtr as an argument; but they only use it to format the domain XML and send it to the network hook script. Technically there's nothing preventing a function in the network driver from accessing every attribute of the domain, or even modifying it :-O, that doesn't mean we should do it though.
I'm trying to completely recall a vague memory of something similar to this that happened in the past - something that was needed in multiple hypervisors (which would imply that it should live either in util or conf), but that also needed to call a network function (or maybe some other driver, I forget). When trying to maintain some sort of separation and rules of engagement between the various components, there tend to be cases that just don't fit within the mold.
In this case, I'm wondering if maybe the duplication can be reduced by creating a function in conf (either domain_conf.c or one of its subsidiaries) that takes a *function as an argument and calls that function for each interface. Something like this:
int virDomainGetDHCPInterfaces(virDomainDefPtr def, virDomainDefGetDHCPLeasesCB getLeases, virDomainInterfacePtr **ifaces) {
for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i];
if (virDomainNetDefGetActualType(net) != VIR_DOMAIN_NET_TYPE_NETWORK) continue; if ((n_leases = getLeases(net->data.network.name, net->mac, &leases)) < 0) { OH NOES!!!!! goto error; if (n_leases) { bobloblawlawblog.com .... }
etc etc. }
various cleanup stuff etc.
}
The function getLeases would be a thin (if any) wrapper around a function in the network driver called networkGetDHCPLeases(). The toplevel function in qemu and libxl would then be simple a bit of glue followed by a call to virDomainGetDHCPInterfaces() with a pointer to the appropriate getLeases function.
This way we would eliminate almost all of the duplicate code (most would go into domain_conf.c, and a bit into bridge_driver.c) without needing to teach the network driver about the internal workings of a domain def.
Does that make any sense? Had a look at this and tried, seems hard to put into domain_conf.c: except for the vm->def->nets, almost all the other things are called from src/libvirt-domain.c or src/libvirt-network.c, not only the getLeases cb of a specific network, but even the virDomainInterfacePtr itself is defined in libvirt-domain.h and also its Free function (in case of error, virNetworkDHCPLeaseFree and virDomainInterfaceFree are also needed). Ideas? Hrm, maybe just go with the small amount of copied code? :-)
When originally looking at the patch, I thought it might be quite disruptive to factor it out into something that could be used by both drivers. Unless others have a clever idea, I'm leaning towards pushing this patch as is.
Yeah, I sadly agree with you :-(. There are just too many things connected to it to make it easy to put in a category, and there's enough violations of the boundaries/referencing directions between conf, util, network driver, and hypervisor drivers already; I'd feel better about reducing them rather than increasing them. It's good that you're thinking about it though :-)