[libvirt] patch option needs clarification
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. 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. All development and testing was done with 0.10.2 libvirt src.rpm on Fedora 17. The patch will be submitted based on git. Gene
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.
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.
All development and testing was done with 0.10.2 libvirt src.rpm on Fedora 17.
The patch will be submitted based on git.
I have checked and the patch applies clean to the v0.10.2-maint branch but has problems with the top level. Is providing the patch against the v0.10.2-maint branch adequate or do you want it reworked (does not look like a big deal) to the top level? Gene
On 10/22/2012 11:24 AM, Gene Czarcinski 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.
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.
All development and testing was done with 0.10.2 libvirt src.rpm on Fedora 17.
The patch will be submitted based on git.
I have checked and the patch applies clean to the v0.10.2-maint branch but has problems with the top level. Is providing the patch against the v0.10.2-maint branch adequate or do you want it reworked (does not look like a big deal) to the top level?
It's much simpler if it applies to the head of master. Try doing this: 1) "git log" and grab the commit ID 2) git checkout master 3) git pull 4) git checkout -b newbranch (or whatever you want to call it) 5) git cherry-pick ${commit-id} This may give you a clean merge (at which point you can just "git send-email -1") or it may give some conflicts. These conflicts will be marked in the source file with: <<<<<<<<< code on current branch ========= conflicting code from cherry-picked patch
>>>
do a hand merge of the differences, then run "git commit". You should now have a properly merged commit - do "make check && make syntax-check" then git send-email -1.
On 10/22/2012 01:36 PM, Laine Stump wrote:
On 10/22/2012 11:24 AM, Gene Czarcinski 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.
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.
All development and testing was done with 0.10.2 libvirt src.rpm on Fedora 17.
The patch will be submitted based on git.
I have checked and the patch applies clean to the v0.10.2-maint branch but has problems with the top level. Is providing the patch against the v0.10.2-maint branch adequate or do you want it reworked (does not look like a big deal) to the top level? It's much simpler if it applies to the head of master.
Try doing this:
1) "git log" and grab the commit ID
2) git checkout master
3) git pull
4) git checkout -b newbranch (or whatever you want to call it)
5) git cherry-pick ${commit-id}
This may give you a clean merge (at which point you can just "git send-email -1") or it may give some conflicts. These conflicts will be marked in the source file with:
<<<<<<<<< code on current branch ========= conflicting code from cherry-picked patch do a hand merge of the differences, then run "git commit". You should now have a properly merged commit - do "make check && make syntax-check" then git send-email -1.
OK, I believe I have something and will send it to the list shortly. I am not sure of the procedure you suggested. I tried to cherry-pick my commit onto a branch of master ... way too many errors. I tried to cherry-pick your commit onto a branch of v0.10.2-maint with my patch applied ... way too many errors. Prior to seeing your message I had created a small patch which applied your changes on top of my patch. This worked. So, I "faked it." Created a branch off v0.10.2-maint and applied the patches [mine and my version of yours]. Tarballed the changed files (there are not that many). Create a new branch off master. Restore the tarball. Git cannot tell what order things were done so everybody is happy. I am sure this made you cringe but, well, it worked. Patch to list "real soon now" [I really have to learn more on how to use git] Gene
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).
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).
All development and testing was done with 0.10.2 libvirt src.rpm on Fedora 17.
The patch will be submitted based on git.
Gene
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
On 10/22/2012 03:17 PM, Laine Stump wrote: this way ... just too much could change ... better to start off clean with a restart (that is from the dnsmasq perspective).
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 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. OK, over to you ... what do you want me to do? Gene
On 10/22/2012 03:59 PM, Gene Czarcinski 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
On 10/22/2012 03:17 PM, Laine Stump wrote: 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.
On 10/22/2012 04:25 PM, Laine Stump wrote:
On 10/22/2012 03:59 PM, Gene Czarcinski wrote:
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?
Ah, I just saw that you've already sent the patch, and it *wasn't* on top of the patch that adds --interface. An alternate path would be to have the "switch to conf file" patch first (but *not* adding the --interface option), then remaking that patch to only add to the conf file (ie to be applied *after* this patch). Either way, we need to have them in two separate patches.
On 10/22/2012 04:32 PM, Laine Stump wrote:
On 10/22/2012 03:59 PM, Gene Czarcinski wrote:
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? Ah, I just saw that you've already sent the patch, and it *wasn't* on top of the patch that adds --interface. An alternate path would be to have the "switch to conf file" patch first (but *not* adding the --interface option), then remaking that patch to only add to the conf file (ie to be applied *after* this patch). Either way, we need to have
On 10/22/2012 04:25 PM, Laine Stump wrote: them in two separate patches.
That first patch was crap and I wish I could have retracted the message after I sent it. "v2" of the patch is "on the way" if not already posted. This patch was created by: 1. checkout master; pull, checkout -b gc-cf-4 master 2. Using "patch -p1", apply my patch (no interface=) for bridge_driver.[ch], and networkxml2argvtest.c 3. Edit bridge_driver.c to fixup the things that did no go on clean. 4. create tarball from another tree for the *argv testfiles and untar onto gc-cf-4 5. commit and the format patch 6. create another branch from master and apply the created patch; fixup a couple end-of-line whitespace problem; reapply ... this time clean. 7 send-email Well, one good thing is that I am starting to get the hang of git ;) This version of the patch did not screw things up in bridge_driver.c like the last one did. Yes, this version does not include the "interface=" code which involves a couple of lines in bridge_driver.c plus an update to each of the argv test files. A patch adding "interface=" to the previous patch will be submitted shortly. I would appreciate an explanation why there is reluctance to adding "interface=". Yes, there were problems a while ago if it was used, but there is now a definite problem if it is not specified and dnsmasq =>2.61. Without "interface=", the bind-interfaces does not work and v4 and/or v6 packets can be mis-routed by the kernel when there are multiple instances of dnsmasq running. Dnsmasq listens to 0.0.0.0:67/68 for v4 and :::547 for v6. Without good packet routing results are unpredictable. Gene
On 10/22/2012 03:59 PM, Gene Czarcinski 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
On 10/22/2012 03:17 PM, Laine Stump wrote: 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).
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 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.
OK, over to you ... what do you want me to do?
I just noticed something "strange" about the patch I submitted. Wait a bit while I look into what is going on. Gene
participants (2)
-
Gene Czarcinski -
Laine Stump