[libvirt] [PATCH] libxl: add .domainInterfaceAddresses
Laine Stump
laine at laine.org
Wed May 25 14:52:18 UTC 2016
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 at laine.org>, Laine Stump <laine at 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 at suse.com>, Jim Fehlig
>>>>> <jfehlig at 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 at 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 :-)
More information about the libvir-list
mailing list