[libvirt] [PATCH] libxl: add .domainInterfaceAddresses
Laine Stump
laine at laine.org
Tue May 17 15:46:41 UTC 2016
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?
(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.)
More information about the libvir-list
mailing list