[libvirt] patch option needs clarification

[Laine Stump]

On 10/22/2012 03:59 PM, Gene Czarcinski wrote:
> On 10/22/2012 03:17 PM, Laine Stump wrote:
>> On 10/22/2012 09:26 AM, Gene Czarcinski wrote:
>>> I am pretty much complete creating a patch which changes how dnsmasq
>>> is started by moving the command line parameters into a conf file.
>>> This new file is placed into the same directory and the lease file.
>>>
>>> The test for the command line arguments now checks the contents of the
>>> conf-file and there is no longer any tests for the command line
>>> parameters which are now two.
>> This will also change the requirements of whether or not a dnsmasq
>> restart or "refresh" (SIGHUP) is required when certain pieces of the
>> network definition are changed with virNetworkUpdate(), but that can be
>> submitted as a separate patch. (basically, things that used to require a
>> change on the commandline, but now only require a change in the conf
>> file, will no longer require a restart).
> SIGHUP will not do it ... I check into it.  By design, configuration
> files are not revisited once started.  Some other files are but not
> the configuration files.  It will take a restart whether is is on the
> command line or in a conf-file.  I believe I understand why it is done
> this way ... just too much could change ... better to start off clean
> with a restart (that is from the dnsmasq perspective).

Interesting. I'm glad you investigated - I would have assumed (*did*
assume) the opposite.

>>
>>>
>>> The first command line parameter is (naturally) --conf-file=<filename>.
>>>
>>> The second parameter adds new functionality and is
>>> --conf-dir=<directory>.  This directory is placed into the same
>>> directory as the conf-file and the lease file.  The name of this
>>> directory is "<net-name>.d". This was added to make testing/debugging
>>> of new dnsmasq options easier since it no longer requires rebuilding
>>> the binaries.  This is also useful for adding log-dhcp and/or
>>> log-queries for a specific network.
>>>
>>> Now the option question.  I can submit the patch assuming the my
>>> previously submitted patch to add --interface to the command line has
>>> been applied or I can assume that it has not been applied.  In either
>>> case, the new code adds a interface=<dev-name> to the conf-file.
>> Submit it assuming that the patch *has* been applied. I think we do need
>> to make that change (adding "--interface"), but it needs to be
>> explicitly visible in a separate patch (and we need to do some testing
>> before pushing it).
> Oops.  But then I am not sure how to do it.  The new code changes
> "everything."
>
> If you want, I can go back and remove it ... make yet-another-patch on
> top of the one one to way to the list which removes interface= from
> the conf-file and then submit a third patch which puts it back in.

I think we're crossing wires. This is what patches I think should be sent:

1) a patch to add --interface to the commandline

2) a patch to switch from using the "long commandline" to using a conf
file (which will still put the equivalent of --interface=xxx into the
conf file).

Isn't that what you already have?

When we push, I think we should push both of these patches, so that
there is a separate record of adding --interface, and another of
switching to using a conf file. If the changes are separated and a
problem is later found, we can more easily use git bisect to determine
the exact change that caused failure; if you sneak other changes into
the "switch to conf file" patch besides simply "switching to a conf
file", then it will be more difficult to determine whether the problem
was caused by that switch, or by changing what options are being given
to dnsmasq.



>
> I am not sure I understand the reluctance about putting the
> --interface patch in.  This problem exists in using any dnsmasq =>
> 2.61.  I believe I have seen it happen although I did not realize it
> at the time.  Simon says that there is logging of the event in
> dnsmasq's rfc3315.c but I have not gone back to prove that the problem
> exists if --interface is not specified.  I am not talking about adding
> this to older versions of libvirt ... just the current one and the
> version that is on Fedora 17 since dnsmasq was so quickly updated to 2.63.

The problem is with people people building the newer libvirt on older
systems.
distro package maintainers are not the only people using upstream
libvirt source.

>
>
> OK, over to you ... what do you want me to do?

I'm assuming the patch you have that switches to using a conf file is of
changes made on top of the change to use --interface, in which case
that's what you should send. Otherwise I'm fairly certain it will be a
simple merge conflict that's easily resolvable when you do the git
rebase/cherry-pick, or however you combine the two patches onto a single
branch. If you're unfamiliar/uneasy doing this, just send the patches
you have; I'll do the merge, then send you back the steps I used so that
you'll have something to work with the next time something similar happens.