Re: [libvirt] [PATCH] libxl: add .domainInterfaceAddresses

>>> On 5/17/2016 at 11:46 PM, in message <2fa0cb6f-ea83-d6f3-18f8-51a671574205(a)laine.org&gt;, Laine Stump <laine(a)laine.org&gt; 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(a)suse.com&gt;, Jim Fehlig >>> <jfehlig(a)suse.com&gt; 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(a)suse.com&gt; >>>>> --- >>>>> 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?