>>> On 5/17/2016 at 11:46 PM, in message
<2fa0cb6f-ea83-d6f3-18f8-51a671574205@xxxxxxxxx>, Laine Stump <laine@xxxxxxxxx>
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@xxxxxxxx>, Jim Fehlig
> >> <jfehlig@xxxxxxxx> 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@xxxxxxxx>
> >>>> ---
> >>>> 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?
-Chunyan
>
> (NB: for any "super double secret API" network driver function you
> implement, you need to implement and alternative NOP function that is
> used when the bridge driver is disabled. See bridge_driver.h:68 for
> examples.)
>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list