On Thu, Jan 06, 2022 at 02:24:18PM -0500, Laine Stump wrote:
On 1/4/22 7:57 AM, Daniel P. Berrangé wrote:
> On Sun, Jan 02, 2022 at 09:41:37PM -0500, Laine Stump wrote:
> > I'm currently working on switching the backend of the network driver from
> > using iptables to using nftables. Due to some functionality that is not
> > available with nftables (the rule that fixes up the checksum of DHCP packets
> > which, btw, is only relevant for *very* old guests, e.g. RHEL5), this needs
> > to be opt-in via a config file setting. In the meantime, in order to make
> > this doable in a reasonable amount of time, I am *not* converting the
> > nwfilter driver right away, and when I do it will need its own config file
> > setting for opt-in.
> >
> > I've never before looked at the code for the .conf file settings at all. I
> > had assumed there would be some sort of "pull" API, where code in
the
> > drivers could call, e.g. virConfGetString("filter_backend") and it
would
> > return the config setting to the caller. But when I look at it, I see that
> > all daemons use the same daemonConfigLoadFile() called from
> > remote_daemon.c:main() (which is shared by all the daemons) and the
> > daemonConfig object that is created to hold the config settings that are
> > read is only visible within main() - the only way that a config setting is
> > used is by main() "pushing" it out to a static variable somewhere
else where
> > it is later retrieved by the interested party, e.g. the way that main()
> > calls daemonSetupNetDevOpenvswitch(config), which then sets the static
> > virNetDevOpenvswitchTimeout in util/virnetdevopenvswitch.c.
>
> I'd consider the OVS approach to be a bad example
>
> Global state needing configurable parameters for stuff in util/ should
> generally be considered a design flaw.
Okay, so then the setting of the host uuid is also a bad example (set into a
static in util/viruuid.c), as is all the config for logging (set in statics
in util/virlog.c by calling a function in util/virdaemon.c) :-)
Ok true, there are reasonable exceptions like logging / uuid.
> ie ovs_timeout should have been in qemu.conf (any other
drivers' config
> files if appropriate).
Somehow I had always considered qemu.conf to be specifically for things
related to starting the qemu process *only* (and not necessarily pertaining
to the entire qemu driver), although even with that interpretation I guess
ovs_timeout could be considered to be in that group as well (since it's used
when running ovs-vsctl as part of preparing the network connection for a
qemu process that will soon be started). I see now I've just been too narrow
minded all this time.
I think I'd describe 'libvirtd.conf' as being for settings related to
operation of the daemon / RPC system, while 'qemu.conf' is for settings
related to operation of the QEMU driver or any features it uses.
With this view point logging / host uuid is appropriate for libvirtd.conf
but OVS timeouts, firewall settings are driver conf things
> This stuff even gets into the libvirt.so that's used client
side,
> so the argument that we had a single monolithic libvirtd didn't
> apply either.
Really? I have always just assumed that if nothing in a particular .o was
referenced, then that .o wouldn't show up in the binary. And even if that
isn't the case, then we could tailor the build to only include those sources
that are actually used (although that would be cumbersome to maintain).
What you describe is true if your linking to a static library.
The src/util stuff does indeed get into a static libvirt_util.a library,
but this is then built into a dynamic library libvirt.so and all the
APIs in src/util are exported (with @LIBVIRT_PRIVATE version tag).
As a result no .o files in src/util will ever get dropped.
> > If I could count on all builds using split daemons (i.e.
separate
> > virtnetworkd and virtnwfilterd) then I could add a similar API in
> > virfirewall.c that remote_daemon.c:main() could use to set
"filter_backend"
> > into a static in virfirewall.c (which is used by both drivers) and
> > everything would just happily work:
> >
> > virtnetworkd.conf:
> > filter_backend = nftables
> >
> > virtnwfilterd.conf
> > filter_backend = iptables
>
> Putting these settings into virtnetworkd.conf and virtnwfilterd.conf
> certainly makes conceptual sense.
Or maybe, based on what I say about "virtqemud.conf vs. qemu.conf" (and thus
"virtnetworkd.conf vs. network.conf") above, they should be put in
networkd.conf and nwfilter.conf. (again, I'm loathe to create "yet
another"
config file, but that seems the most logical thing to do).
Sorry, yes, I mis-typed. virtnetworkd.conf / virtnwfilterd.conf are
the direct equivalent of libvirtd.conf so I shouldn't have written
that. I meand network.conf / nwfilter.conf which would be driver
config files, not daemon config files.
Regards,
Daniel
--
|:
https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
|:
https://libvirt.org -o-
https://fstop138.berrange.com :|
|:
https://entangle-photo.org -o-
https://www.instagram.com/dberrange :|