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 :|
