Re: [libvirt] [PATCHv3] Add forwarder attribute to <dns /> element.

On 09/13/2013 10:31 AM, Diego Woitasen wrote: s/resolv /resolve / Yay for docs and tests alongside the feature. Failed 'make syntax-check' trailing_blank src/conf/network_conf.c:2301: if (!(def->forwardPlainNames || def->forwarders || def->nhosts || tests/networkxml2confdata/nat-network-dns-forwarders.xml:6: <dns> maint.mk: found trailing blank(s) s,N/A,1.1.3, Even if we don't know the next release number, it's always safe to do current release + 1; if we decide the next release will be renumbered to 1.2.0, we do a search-and-replace at that time; it's easier to replace actual numbers than to search for N/A. Also, long lines (it doesn't hurt to wrap to keep within 80 columns); although you were just copying other long lines in the area. Pre-existing, but this is is missing an <interleave>. We prefer allowing the user to do either: <dns> <txt ...> <forwarder ...> </dns> or <dns> <forwarder ...> <txt ...> </dns> since there is no logical connection that requires them to come in a particular order. And while fixing that, I also noticed that the pre-existing indentation is screwy. Unusual ordering; it would be nice to always have fwds after txt, instead of sometimes before. No space before /> in generated output. Also, this outputs <forwarders>, but the parser and RNG expect <forwarder>. Did you test this? Indentation is off. Spaces around '=' in C code. No space before /> Ah, I see why you missed some of this problems - you didn't enhance networkxml2xmltest.c. [Ugh - why are we repeating files verbatim between networkxml2confdata, networkxml2xmlin, and networkxml2xmlout? We should fix that up in a separate patch to copy how qemu xml tests have been streamlined to assume out matches in if no out file exists, and to use confdata for input tests for both xml2xml and xml2conf]. ACK with the following squashed in, so I will push shortly. diff --git c/docs/formatnetwork.html.in i/docs/formatnetwork.html.in index feb6895..3d57ac1 100644 --- c/docs/formatnetwork.html.in +++ i/docs/formatnetwork.html.in @@ -688,10 +688,12 @@ Currently supported sub-elements of <code>&lt;dns&gt;</code> are: <dl> <dt><code>forwarder</code></dt> - <dd>A <code>dns</code> element can have 0 or more <code>forwarder</code> elements. - Each forwarder element defines an IP address to be used as forwarder - in DNS server configuration. The addr attribute is required and defines the - IP address of every forwarder. <span class="since">Since N/A</span> + <dd>A <code>dns</code> element can have 0 or + more <code>forwarder</code> elements. Each forwarder + element defines an IP address to be used as forwarder in + DNS server configuration. The addr attribute is required + and defines the IP address of every + forwarder. <span class="since">Since 1.1.3</span> </dd> <dt><code>txt</code></dt> <dd>A <code>dns</code> element can have 0 or more <code>txt</code> elements. diff --git c/docs/schemas/network.rng i/docs/schemas/network.rng index 4cc3af8..0e7da89 100644 --- c/docs/schemas/network.rng +++ i/docs/schemas/network.rng @@ -242,6 +242,7 @@ </choice> </attribute> </optional> + <interleave> <zeroOrMore> <element name="forwarder"> <attribute name="addr"><ref name="ipAddr"/></attribute> @@ -256,13 +257,21 @@ <zeroOrMore> <element name="srv"> <attribute name="service"><text/></attribute> - <attribute name="protocol"><ref name="protocol"/></attribute> + <attribute name="protocol"> + <ref name="protocol"/> + </attribute> <optional> <attribute name="domain"><ref name="dnsName"/></attribute> <attribute name="target"><text/></attribute> - <attribute name="port"><ref name="unsignedShort"/></attribute> - <attribute name="priority"><ref name="unsignedShort"/></attribute> - <attribute name="weight"><ref name="unsignedShort"/></attribute> + <attribute name="port"> + <ref name="unsignedShort"/> + </attribute> + <attribute name="priority"> + <ref name="unsignedShort"/> + </attribute> + <attribute name="weight"> + <ref name="unsignedShort"/> + </attribute> </optional> </element> </zeroOrMore> @@ -274,6 +283,7 @@ </oneOrMore> </element> </zeroOrMore> + </interleave> </element> </optional> <optional> diff --git c/src/conf/network_conf.c i/src/conf/network_conf.c index 0ba843b..6968e25 100644 --- c/src/conf/network_conf.c +++ i/src/conf/network_conf.c @@ -1044,7 +1044,7 @@ virNetworkDNSDefParseXML(const char *networkName, xmlNodePtr *txtNodes = NULL; xmlNodePtr *fwdNodes = NULL; char *forwardPlainNames = NULL; - int nfwds, nhosts, nsrvs, ntxts; + int nhosts, nsrvs, ntxts, nfwds; size_t i; int ret = -1; xmlNodePtr save = ctxt->node; @@ -2315,7 +2315,7 @@ virNetworkDNSDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); for (i = 0; i < def->nfwds; i++) { - virBufferAsprintf(buf, "<forwarders addr='%s' />\n", + virBufferAsprintf(buf, "<forwarder addr='%s'/>\n", def->forwarders[i]); } diff --git c/src/network/bridge_driver.c i/src/network/bridge_driver.c index a2cfb35..8787bdb 100644 --- c/src/network/bridge_driver.c +++ i/src/network/bridge_driver.c @@ -710,7 +710,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, if (network->def->dns.forwarders) { virBufferAddLit(&configbuf, "no-resolv\n"); - for (i=0; i < network->def->dns.nfwds; i++) { + for (i = 0; i < network->def->dns.nfwds; i++) { virBufferAsprintf(&configbuf, "server=%s\n", network->def->dns.forwarders[i]); } diff --git c/tests/networkxml2confdata/nat-network-dns-forwarders.xml i/tests/networkxml2confdata/nat-network-dns-forwarders.xml index eebec97..8fab78e 100644 --- c/tests/networkxml2confdata/nat-network-dns-forwarders.xml +++ i/tests/networkxml2confdata/nat-network-dns-forwarders.xml @@ -2,10 +2,10 @@ <name>default</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid> <forward dev='eth0' mode='nat'/> - <bridge name='virbr0' stp='on' delay='0' /> + <bridge name='virbr0' stp='on' delay='0'/> <dns> - <forwarder addr='8.8.8.8' /> - <forwarder addr='8.8.4.4' /> + <forwarder addr='8.8.8.8'/> + <forwarder addr='8.8.4.4'/> </dns> <ip address='192.168.122.1' netmask='255.255.255.0'> </ip> diff --git c/tests/networkxml2xmlin/nat-network-dns-forwarders.xml i/tests/networkxml2xmlin/nat-network-dns-forwarders.xml new file mode 100644 index 0000000..eebec97 --- /dev/null +++ i/tests/networkxml2xmlin/nat-network-dns-forwarders.xml @@ -0,0 +1,12 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid> + <forward dev='eth0' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0' /> + <dns> + <forwarder addr='8.8.8.8' /> + <forwarder addr='8.8.4.4' /> + </dns> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> diff --git c/tests/networkxml2xmlout/nat-network-dns-forwarders.xml i/tests/networkxml2xmlout/nat-network-dns-forwarders.xml new file mode 100644 index 0000000..930a42a --- /dev/null +++ i/tests/networkxml2xmlout/nat-network-dns-forwarders.xml @@ -0,0 +1,14 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid> + <forward dev='eth0' mode='nat'> + <interface dev='eth0'/> + </forward> + <bridge name='virbr0' stp='on' delay='0'/> + <dns> + <forwarder addr='8.8.8.8'/> + <forwarder addr='8.8.4.4'/> + </dns> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> diff --git c/tests/networkxml2xmltest.c i/tests/networkxml2xmltest.c index d04039d..c4fca08 100644 --- c/tests/networkxml2xmltest.c +++ i/tests/networkxml2xmltest.c @@ -108,6 +108,7 @@ mymain(void) DO_TEST("nat-network-dns-srv-record-minimal"); DO_TEST("nat-network-dns-hosts"); DO_TEST("nat-network-dns-forward-plain"); + DO_TEST("nat-network-dns-forwarders"); DO_TEST("nat-network-forward-nat-address"); DO_TEST("8021Qbh-net"); DO_TEST("direct-net"); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org