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

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.