[libvirt] curious about dnsmasq --conf-file=

[Laine Stump]

On 09/30/2012 09:55 AM, Gene Czarcinski wrote:
> I understand why there currently is "--conf-file= " on the dnsmasq
> command line ... you do not want dnsmasq to use anything in
> /etc/dnsmasq.conf.
>
> OK, but dnsmasq is picking up a lot of command line parameters.  Why
> aren't these parameters put into a libvirt specific conf file such as
> is done with radvd?
>
> Is there some reason that only command line parameters are used?  I
> can understand that it could be a matter of style/personal choice by
> the individual that originally did it and, it currently works, so why
> mess with it?  But, if a patch was created which changed things from
> the command line to a conf file, would that be accepted?

I was actually thinking about that just a few days ago. The current
excessive use of the commandline means that for many of the changes to
network config with the new virNetworkUpdate(), we must kill and restart
dnsmasq, rather than just sending it a SIGHUP; this is bound to be more
disruptive.

I'm not sure of the historical reasons for putting (almost) everything
on the commandline, but I for one would have no problem with a patch
that switched to putting as much as possible in a private dnsmasq.conf
file (stored in the same directory as the private hosts files, and named
accordingly, so I guess "/var/lib/libvirt/dnsmasq/${netname}.conf").
This should be done as a separate function that populates a char*; I'm
torn between two recommended courses of action:

1) try to understand (and fix!) the mess of dnsmasq.c, and add a
function there (see the git history of that file to understand what I'm
talking about, in particular commit 9523b3c).

2) mimic networkRadvdConfContents(), and possibly the higher level
networkRadvdConfWrite(). This would be simpler and more straightforward,
but would leave a situation where half of the internals of dnsmasq were
handled in one place and half in another (but then that's really no
worse than the current situation, is it? :-)

In either case, you should then be easily able write tests that create
the conf data in a buffer, and compare that to an existing file on disk.

(oh, and such a change would need to 1) be compatible with whatever
version of dnsmasq is currently shipping for RHEL5 / CentOS5 (because I
think that's probably the oldest version we support) - that would just
require manually checking, as well as behaving properly when upgrading a
a command-line-parameter-using version of libvirt to a conf-file-using
version (I *think* the simplest way to handle that would be to check for
existence of the conf file at the top of networkRefreshDhcpDaemon(), and
call networkRestartDhcpDaemon() if the conf file wasn't there).

>
> BTW, I noticed that there are no tests for what is done with radvd but
> there are tests for the dnsmasq command line options.

Guilty as charged :-/ I should have made such tests when I added radvd
support, but didn't think of it, and nobody caught the omission. If you
want to add such tests when you re-do the dnsmasq tests, I'm sure nobody
would complain :-)


>   These tests would no longer apply and thus be deleted.  Can anyone
> cite an example test for parameters put into a conf file?  Or, have
> some of those tests become OBE?  I must say that I have found the
> tests valuable in seeing and debugging additional command line parameters.

Yes. I've caught innumerable xml parse/format bugs due to all the tests,
and can't imagine any useful level of productivity without them.

I can't think of another test that does *exactly* the same thing, but
really you should be able to just slightly reformat
networkxml2argvdata/*.argv slightly (and maybe rename them to *.conf
instead of *.argv), then call the new networkDnsmasqConfContents()
function rather than networkBuildDhcpDaemonCommandline() +
virCommandToString() in networkxml2argvtest.c (hmm - again, maybe you
want to change all occurences of "argv" to "conf" in all the
directory/file names. And while you're at it, you could add in tests for
the hosts file and "addnhosts" file.