[libvirt] [PATCH] (updated) additional parameters needed for dnsmasq
As I said in a previous message, dnsmasq is forwarding a number of queries upstream that should not be done. There still remains an MX query for a plain name with no domain specified that will be forwarded is dnsmasq has --domain=xxx --local=/xxx/ specified. This does not happen with no domain name and --local=// ... not a libvirt problem. BTW, thanks again to Claudio Bley! ==================================================== diff -uNr libvirt-0.9.11.4.orig/src/network/bridge_driver.c libvirt-0.9.11.4/src/network/bridge_driver.c --- libvirt-0.9.11.4.orig/src/network/bridge_driver.c 2012-06-15 14:23:21.000000000 -0400 +++ libvirt-0.9.11.4/src/network/bridge_driver.c 2012-08-22 12:16:45.263488789 -0400 @@ -490,8 +490,15 @@ */ virCommandAddArgList(cmd, "--strict-order", "--bind-interfaces", NULL); - if (network->def->domain) + if (network->def->domain) { virCommandAddArgList(cmd, "--domain", network->def->domain, NULL); + virCommandAddArgFormat(cmd, "--local=/%s/", network->def->domain); + virCommandAddArgList(cmd, "--domain-needed", "--filterwin2k", NULL); + } + else { /* need to specify local even if no domain specified */ + virCommandAddArg(cmd, "--local=//"); + virCommandAddArgList(cmd, "--domain-needed", "--filterwin2k", NULL); + } if (pidfile) virCommandAddArgPair(cmd, "--pid-file", pidfile); diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/isolated-network.argv libvirt-0.9.11.4/tests/networkxml2argvdata/isolated-network.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/isolated-network.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/isolated-network.argv 2012-08-22 12:20:37.700995728 -0400 @@ -1,4 +1,5 @@ -@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces \ +--local=// --domain-needed --filterwin2k --conf-file= \ --except-interface lo --dhcp-option=3 --no-resolv \ --listen-address 192.168.152.1 \ --dhcp-range 192.168.152.2,192.168.152.254 \ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network.argv libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network.argv 2012-08-22 12:21:24.481703184 -0400 @@ -1,4 +1,5 @@ -@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces \ +--local=// --domain-needed --filterwin2k --conf-file= \ --except-interface lo --listen-address 192.168.122.1 \ --listen-address 192.168.123.1 --listen-address 2001:db8:ac10:fe01::1 \ --listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 \ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-hosts.argv libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-hosts.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-hosts.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-hosts.argv 2012-08-22 12:25:30.378218203 -0400 @@ -1,3 +1,4 @@ @DNSMASQ@ --strict-order --bind-interfaces --domain example.com \ +--local=/example.com/ --domain-needed --filterwin2k \ --conf-file= --except-interface lo --listen-address 192.168.122.1 \ --expand-hosts --addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-srv-record.argv libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-srv-record.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-srv-record.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-srv-record.argv 2012-08-22 12:22:35.471268869 -0400 @@ -1,7 +1,7 @@ @DNSMASQ@ \ --strict-order \ --bind-interfaces \ ---conf-file= \ +--local=// --domain-needed --filterwin2k --conf-file= \ --except-interface lo \ --srv-host=name.tcp.test-domain-name,.,1024,10,10 \ --listen-address 192.168.122.1 \ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv 2012-08-22 12:22:48.422191468 -0400 @@ -1,7 +1,7 @@ @DNSMASQ@ \ --strict-order \ --bind-interfaces \ ---conf-file= \ +--local=// --domain-needed --filterwin2k --conf-file= \ --except-interface lo \ --srv-host=name.tcp.,,,, \ --listen-address 192.168.122.1 \ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-txt-record.argv libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-txt-record.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/nat-network-dns-txt-record.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/nat-network-dns-txt-record.argv 2012-08-22 12:23:47.642834970 -0400 @@ -1,4 +1,5 @@ -@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces \ +--local=// --domain-needed --filterwin2k --conf-file= \ --except-interface lo --txt-record=example,example value \ --listen-address 192.168.122.1 --listen-address 192.168.123.1 \ --listen-address 2001:db8:ac10:fe01::1 \ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/netboot-network.argv libvirt-0.9.11.4/tests/networkxml2argvdata/netboot-network.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/netboot-network.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/netboot-network.argv 2012-08-22 12:24:31.838569611 -0400 @@ -1,5 +1,6 @@ @DNSMASQ@ --strict-order --bind-interfaces --domain example.com \ ---conf-file= --except-interface lo --listen-address 192.168.122.1 \ +--local=/example.com/ --domain-needed --filterwin2k --conf-file= \ +--except-interface lo --listen-address 192.168.122.1 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ --dhcp-lease-max=253 --dhcp-no-override --expand-hosts --enable-tftp \ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/netboot-proxy-network.argv libvirt-0.9.11.4/tests/networkxml2argvdata/netboot-proxy-network.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/netboot-proxy-network.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/netboot-proxy-network.argv 2012-08-22 12:27:24.586499884 -0400 @@ -1,5 +1,6 @@ @DNSMASQ@ --strict-order --bind-interfaces --domain example.com \ ---conf-file= --except-interface lo --listen-address 192.168.122.1 \ +--local=/example.com/ --domain-needed --filterwin2k --conf-file= \ +--except-interface lo --listen-address 192.168.122.1 \ --dhcp-range 192.168.122.2,192.168.122.254 \ --dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ --dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ diff -uNr libvirt-0.9.11.4.orig/tests/networkxml2argvdata/routed-network.argv libvirt-0.9.11.4/tests/networkxml2argvdata/routed-network.argv --- libvirt-0.9.11.4.orig/tests/networkxml2argvdata/routed-network.argv 2012-06-15 14:21:54.000000000 -0400 +++ libvirt-0.9.11.4/tests/networkxml2argvdata/routed-network.argv 2012-08-22 12:28:06.111841912 -0400 @@ -1,2 +1,3 @@ -@DNSMASQ@ --strict-order --bind-interfaces --conf-file= \ +@DNSMASQ@ --strict-order --bind-interfaces \ +--local=// --domain-needed --filterwin2k --conf-file= \ --except-interface lo --listen-address 192.168.122.1\ ========================================== Just in case, I also attached the patch. Gene
On 08/22/2012 10:59 AM, Gene Czarcinski wrote:
As I said in a previous message, dnsmasq is forwarding a number of queries upstream that should not be done. There still remains an MX query for a plain name with no domain specified that will be forwarded is dnsmasq has --domain=xxx --local=/xxx/ specified. This does not happen with no domain name and --local=// ... not a libvirt problem.
This message needs adjustment before it is appropriate for libvirt.git (a year from now, someone reading 'git log' will have no idea what it was '(updated)' from, nor know the URL to the 'previous message').
- if (network->def->domain) + if (network->def->domain) { virCommandAddArgList(cmd, "--domain", network->def->domain, NULL); + virCommandAddArgFormat(cmd, "--local=/%s/", network->def->domain); + virCommandAddArgList(cmd, "--domain-needed", "--filterwin2k", NULL); + } + else { /* need to specify local even if no domain specified */ + virCommandAddArg(cmd, "--local=//"); + virCommandAddArgList(cmd, "--domain-needed", "--filterwin2k", NULL); + }
Simpler as: if (network->def->domain) virCommandArgPair(cmd, "--domain", network->def->domain); virCommandAddArgFormat(cmd, "--local=/%s/", network->def->domain ? network->def->domain : ""); virCommandAddArgList(cmd, "--domain-needed", "--filterwin2k", NULL); with a corresponding tweak in the testsuite to recognize '--domain=example.com' as a result.
Just in case, I also attached the patch.
Thanks, that helped. Your mail failed to make it through 'git am', but your attachment made it through 'git apply' so I was still able to piece the two together to form the commit message using the headers from your email for correct attribution. But using 'git send-email' in the future would make it easier to apply your patches; we have some hints on that in our HACKING file. ACK and pushed with the above tweak, and with adding you to AUTHORS (let us know if you prefer any other spelling or email address; the file is in UTF-8). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/22/2012 11:39 AM, Eric Blake wrote:
On 08/22/2012 10:59 AM, Gene Czarcinski wrote:
As I said in a previous message, dnsmasq is forwarding a number of queries upstream that should not be done. There still remains an MX query for a plain name with no domain specified that will be forwarded is dnsmasq has --domain=xxx --local=/xxx/ specified. This does not happen with no domain name and --local=// ... not a libvirt problem.
ACK and pushed with the above tweak, and with adding you to AUTHORS (let us know if you prefer any other spelling or email address; the file is in UTF-8).
Oh, and now that I've already pushed, I have a high-level question: what is the minimum version of 'dnsmasq' that supports the command-line syntax that this patch introduces? +--local=// --domain-needed --filterwin2k \ If older dnsmasq doesn't recognize --local=// or the new --domain-needed or --filterwin2k options, then we either need to make this code conditional based on probing 'dnsmasq --help' at startup, or else change the spec file to require a larger minimum version of dnsmasq (we already require 2.41 for other reasons). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/22/2012 01:47 PM, Eric Blake wrote:
On 08/22/2012 11:39 AM, Eric Blake wrote:
On 08/22/2012 10:59 AM, Gene Czarcinski wrote:
As I said in a previous message, dnsmasq is forwarding a number of queries upstream that should not be done. There still remains an MX query for a plain name with no domain specified that will be forwarded is dnsmasq has --domain=xxx --local=/xxx/ specified. This does not happen with no domain name and --local=// ... not a libvirt problem.
ACK and pushed with the above tweak, and with adding you to AUTHORS (let us know if you prefer any other spelling or email address; the file is in UTF-8). Oh, and now that I've already pushed, I have a high-level question: what is the minimum version of 'dnsmasq' that supports the command-line syntax that this patch introduces?
+--local=// --domain-needed --filterwin2k \ I honestly do not know for sure ... I am running dnsmasq-2.59 and checking the dnsmasq website shows that the latest is 2.63. The oldest "active" tarball is 2.45 and the example dnsmasq.conf it it has local= domain-needed and filterwin2k
I believe these have been around for some time and no version that libvirt would support will not support them. BTW, I believe that you folks are making the right choice in using the long-names form of the options ... the single character versions can just be too confusing: "-s" and "-S" mean two very different things. "-s" is the same as "--domain" and "-S" is the same as "--server" which is also the same as "--local". I am not absolutely sure that "--filterwin2k" is needed to get rid of the bad/undesired forwarding but it sounded like an option that should be used in any case.
If older dnsmasq doesn't recognize --local=// or the new --domain-needed or --filterwin2k options, then we either need to make this code conditional based on probing 'dnsmasq --help' at startup, or else change the spec file to require a larger minimum version of dnsmasq (we already require 2.41 for other reasons).
I went back and checked some of the older versions of dnsmasq (2.41 and 2.37). local=//, --domain-needed, and --filterwin2k are all present in the example dnsmasq.conf files. This is not to say the the dnsmasq software does exactly the same thing in all versions. According the the CHANGELOG, there was some code "tweaking" for domain-needed in 2.58. I am truly sorry that my patch and email made life a bit difficult for you. I have not delved into any of the virtualization code and your setup & convensions are all reasonable but I just did not know. The last package I hacked around with was NetworkManager and libvirt is a lot different. As far as the code change you made to my patch, both versions work but yours is a bit more eligant. Gene
On 08/22/2012 01:36 PM, Gene Czarcinski wrote:
Oh, and now that I've already pushed, I have a high-level question: what is the minimum version of 'dnsmasq' that supports the command-line syntax that this patch introduces?
I went back and checked some of the older versions of dnsmasq (2.41 and 2.37). local=//, --domain-needed, and --filterwin2k are all present in
That should be good enough. Thanks for the research!
the example dnsmasq.conf files. This is not to say the the dnsmasq software does exactly the same thing in all versions. According the the CHANGELOG, there was some code "tweaking" for domain-needed in 2.58.
May be true, but hopefully we'll get a decent report if someone runs into subtleties caused by this; but at least we know we won't hit anyone complaining that dnsmasq no longer starts due to unknown options.
I am truly sorry that my patch and email made life a bit difficult for you. I have not delved into any of the virtualization code and your setup & convensions are all reasonable but I just did not know. The last package I hacked around with was NetworkManager and libvirt is a lot different.
No problem - we're used to helping out first-time contributors. Open source is successful when you give people the benefit of a doubt, and encourage their contribution in spite of difficulties. On the converse side, touching up every single patch doesn't scale well, which is why we write HACKING documents, and why we try to be friendly even when asking for a re-submission, and why we aren't quite so lenient on HACKING violations from repeated contributors. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/22/2012 11:47 AM, Eric Blake wrote:
On 08/22/2012 11:39 AM, Eric Blake wrote:
On 08/22/2012 10:59 AM, Gene Czarcinski wrote:
As I said in a previous message, dnsmasq is forwarding a number of queries upstream that should not be done. There still remains an MX query for a plain name with no domain specified that will be forwarded is dnsmasq has --domain=xxx --local=/xxx/ specified. This does not happen with no domain name and --local=// ... not a libvirt problem.
ACK and pushed with the above tweak, and with adding you to AUTHORS (let us know if you prefer any other spelling or email address; the file is in UTF-8).
Oh, and now that I've already pushed, I have a high-level question: what is the minimum version of 'dnsmasq' that supports the command-line syntax that this patch introduces?
+--local=// --domain-needed --filterwin2k \
If older dnsmasq doesn't recognize --local=// or the new --domain-needed or --filterwin2k options, then we either need to make this code conditional based on probing 'dnsmasq --help' at startup, or else change the spec file to require a larger minimum version of dnsmasq (we already require 2.41 for other reasons).
Just as I feared, we introduced a regression: https://bugzilla.redhat.com/show_bug.cgi?id=854137 Apparently, --filterwin2k disables features needed by Windows guests. Gene, what is the benefit vs. cost of adding this flag? I'm trying to figure out whether we need to expose it as something user-configurable, or whether we should just revert back to the pre-patch version that did not supply that option. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/04/2012 11:12 AM, Eric Blake wrote:
On 08/22/2012 11:47 AM, Eric Blake wrote:
On 08/22/2012 11:39 AM, Eric Blake wrote:
On 08/22/2012 10:59 AM, Gene Czarcinski wrote:
As I said in a previous message, dnsmasq is forwarding a number of queries upstream that should not be done. There still remains an MX query for a plain name with no domain specified that will be forwarded is dnsmasq has --domain=xxx --local=/xxx/ specified. This does not happen with no domain name and --local=// ... not a libvirt problem.
ACK and pushed with the above tweak, and with adding you to AUTHORS (let us know if you prefer any other spelling or email address; the file is in UTF-8). Oh, and now that I've already pushed, I have a high-level question: what is the minimum version of 'dnsmasq' that supports the command-line syntax that this patch introduces?
+--local=// --domain-needed --filterwin2k \
If older dnsmasq doesn't recognize --local=// or the new --domain-needed or --filterwin2k options, then we either need to make this code conditional based on probing 'dnsmasq --help' at startup, or else change the spec file to require a larger minimum version of dnsmasq (we already require 2.41 for other reasons). Just as I feared, we introduced a regression:
https://bugzilla.redhat.com/show_bug.cgi?id=854137
Apparently, --filterwin2k disables features needed by Windows guests. Gene, what is the benefit vs. cost of adding this flag? I'm trying to figure out whether we need to expose it as something user-configurable, or whether we should just revert back to the pre-patch version that did not supply that option.
I already had some second thoughts about --filterwin2k but you had pushed it. "--filterwin2k" should be removed. Unfortunately, as I said in another message, this "--local=" and "--domain-needed" are required but are not complete. dnsmasq will forward PTR queries which fall in the dnsmasq domain. I need to figure out how to create something of the form "--local=/xxx.yyy.zzz.in-addr.arpa/". I have tested what the effect of this is and it works. Gene
On 09/05/2012 07:55 AM, Gene Czarcinski wrote:
On 09/04/2012 11:12 AM, Eric Blake wrote:
On 08/22/2012 11:47 AM, Eric Blake wrote:
On 08/22/2012 11:39 AM, Eric Blake wrote:
On 08/22/2012 10:59 AM, Gene Czarcinski wrote:
As I said in a previous message, dnsmasq is forwarding a number of queries upstream that should not be done. There still remains an MX query for a plain name with no domain specified that will be forwarded is dnsmasq has --domain=xxx --local=/xxx/ specified. This does not happen with no domain name and --local=// ... not a libvirt problem.
ACK and pushed with the above tweak, and with adding you to AUTHORS (let us know if you prefer any other spelling or email address; the file is in UTF-8). Oh, and now that I've already pushed, I have a high-level question: what is the minimum version of 'dnsmasq' that supports the command-line syntax that this patch introduces?
+--local=// --domain-needed --filterwin2k \
If older dnsmasq doesn't recognize --local=// or the new --domain-needed or --filterwin2k options, then we either need to make this code conditional based on probing 'dnsmasq --help' at startup, or else change the spec file to require a larger minimum version of dnsmasq (we already require 2.41 for other reasons). Just as I feared, we introduced a regression:
https://bugzilla.redhat.com/show_bug.cgi?id=854137
Apparently, --filterwin2k disables features needed by Windows guests. Gene, what is the benefit vs. cost of adding this flag? I'm trying to figure out whether we need to expose it as something user-configurable, or whether we should just revert back to the pre-patch version that did not supply that option.
I already had some second thoughts about --filterwin2k but you had pushed it. "--filterwin2k" should be removed.
Yes, as rare as dialup lines are these days, I think it's highly unlikely that anyone running a virt host will be connected to the rest of the network in a way which will require bringing up a dialup network connection in order to send a packet to a domain controller. So, I don't think we should clutter the XML with such a specific option that will in all likelyhood never be used.
participants (3)
-
Eric Blake -
Gene Czarcinski -
Laine Stump