Hi Laine,
thanks for having a look!
On Tue, Feb 02, 2016 at 01:35:28PM -0500, Laine Stump wrote:
On 01/31/2016 01:42 PM, Guido Günther wrote:
>so we can use it from the LXC driver as well.
>---
>I couldn't find a nice place to add this so I went for a separate file. I'm
>happy to move this elsewhere.
A separate file is fine, but it can't be in the util directory, because
you're including a file in the conf directory (virdomainobjlist), and files
in util aren't allowed to reference files in conf (ugh, I see we're doing it
in several places anyway; I thought those had all been cleared out; I know
we at least *discussed* it). Also the function is calling public libvirt API
functions (virNetworkLookupByName and virNetworkGetDHCPLeases,
virNetworkDHCPLeaseFree, virDomainInterfaceFree) and using datatypes from
the public API, also not allowed in the util directory.
It kind of occurred to me while moving code around that s.th. like this
might happen but I couldn't come up with good reasons (that wouldn't
allow for exceptions), I couldn't find anything in the contribution
guidelines ether why we wouldn't in util/
* use files from conf
* use public API
except for keeping things self contained (i.e. to avoid deadlocks).
When faced with a similar problem several years back and not seeing
a
reasonable alternative, I "temporarily" created backdoor functions into the
network driver
(networkAllocateActualDevice/networkNotifyActualDevice/networkReleaseActualDevice)
which are called directly from the hypervisor drivers to allocate physical
ethernet devices from the pools of devices managed by the network driver.
This is problematic because a stub function needs to be provided in case the
network driver isn't built, and also because it creates a hard dependency
for daemon-driver-network in daemon-driver-qemu and daemon-driver-lxc when
the network driver *is* built. I actually expected that it would be shot
down in review and I'd have to come up with something else (or, even better,
that someone would suggest a cleaner alternative), but it was acked and has
been in place since 2011 or so with apparently no complaints (commit
04711a0f3).
But of course you have a function that trawls through the domain list as
well, so it's not really appropriate to have it in the network driver
either, so...
I wonder if it wouldn't be simpler to just c'n'p the function into the
LXC driver then? I didn't do so in the first place since other drivers
could benefit from this as well?
Cheers,
-- Guido