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

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