Sorry for the long delay in responding to these patches. Unfortunately, they will no longer apply with "git am", so it's difficult to look at them "in-tree", but I think they need some revision anyway :-) On 04/01/2011 06:45 AM, Michal Novotny wrote:

Make: passed Make check: passed Make syntax-check: passed

Hi, this is the patch to add support for adding TXT records to the DNS service running on the virtual network. This has been tested on Fedora-14 i386 box and tests are also added to RelaxNG schema and test XML files.

Since this comment will appear in the commit logs for all posterity, the information about passing make check etc shouldn't really be here (that should be implied by the fact that it's committed! :-) Also, the "this is the patch" part and telling the exact setup of the testing are things useful for annotations in the email, but not when pushing the patch, so are more appropriate for the 0/5 email. Conversely, in your 0/5 introduction, you gave an example of the new XML - that would be useful in this commit comment so that someone could easily search the git log to learn when support was added for this feature.

Since spaces are not allowed for the TXT records in DNS they are rejected

and "TXT records in DNS doesn't support spaces" error message is being output to the user.

You don't really need to say the exact log message in the commit comment

It's been tested and checked/syntax-checked and everything was working fine.

Again, the sentence above should be in the introductory email, but is superfluous here.

Also, the formatnetwork HTML document has been altered to include those information about new DNS tag.

"Also the network XML documentation has been updated to describe the new <dns> element."

Michal

Again, signature not needed in a commit comment - it's implied in the email address.

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- docs/formatnetwork.html.in | 24 ++++++- docs/schemas/network.rng | 12 +++ src/conf/network_conf.c | 71 ++++++++++++++++++++ src/conf/network_conf.h | 16 +++++ src/network/bridge_driver.c | 15 ++++- .../nat-network-dns-txt-record.xml | 24 +++++++ .../nat-network-dns-txt-record.xml | 24 +++++++ tests/networkxml2xmltest.c | 1 + 8 files changed, 185 insertions(+), 2 deletions(-) create mode 100644 tests/networkxml2xmlin/nat-network-dns-txt-record.xml create mode 100644 tests/networkxml2xmlout/nat-network-dns-txt-record.xml

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index c6969eb..5211ed2 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -108,7 +108,10 @@ The final set of elements define the addresses (IPv4 and/or IPv6, as well as MAC) to be assigned to the bridge device associated with the virtual network, and optionally enable DHCP - services. + services. The network creation also supports the TXT record in + the DNS to expose some information to the guest using this + record. This feature could be used in the similar way like DKIM + uses TXT records of DNS to expose public key. </p>

That needs some rewording, but should also be moved into its own paragraph (see my next comment) anyway. Possibly it will end up being something like: dns The dns element of a network contains configuration information for the virtual network's DNS server. <span class="since">Since 0.9.1</span> Currently supported elements are: txt (note that I am changing the name from "txt-record" to "txt" - I think the "record" is kind of implied) A dns element can have 0 or more txt elements. Each txt element defines a DNS TXT record and has two attributes, both required: a name that can be queried via dns, and a value that will be returned when that name is queried. names cannot contain embedded spaces or commas. value is a single string that can contain multiple values which are comma-separated. (Does each value create a separate TXT record with the same name, or are they all together really a single value that happens to have commas? I ask this because if it is the former, the implementation may differ on other DNS servers - we want to avoid embedding idiosyncracies of the dnsmasq implementation into our design!)

<pre> @@ -120,6 +123,9 @@ <host mac="00:16:3e:77:e2:ed" name="foo.example.com" ip="192.168.122.10" /> <host mac="00:16:3e:3e:a9:1a" name="bar.example.com" ip="192.168.122.11" /> </dhcp> +<dns> +<txt-record name="example" value="example value" /> +</dns>

Since there is a single instance of dnsmasq listening on all IPs defined for a network, and the txt records will be visible to all of them, I think the DNS section should be one level up in the tree - at the same level as IP rather than below it, ie: <network> <name>default</name> <dns> <txt name="example" value="example value" /> </dns> ... </network>

</ip> </network></pre>

@@ -199,6 +205,22 @@ element is used. The BOOTP options currently have to be the same for all address ranges and statically assigned addresses.<span class="since">Since 0.7.1 (<code>server</code> since 0.7.3).</span> +</dd><dt><code>dns</code></dt><dd>Also within the<code>ip</code> element + there is an optional<code>dns</code> element. The presence of this element + enables configuration and exposal of records in the DNS service on the + virtual network. It will further contain one or more<code>txt-record</code> + elements. The<code>dns</code> element is supported for both IPv4 and IPv6 + networks.<span class="since">Since 0.9.1</span> +</dd> +<dt><code>txt-record</code></dt> +<dd>The<code>txt-record</code> element is the definition of TXT record for the + DNS service. There are two attributes that both have to be used for the TXT + record definition:<code>name</code> and<code>value</code>. The<code>name +</code>attribute doesn't support commas in it's value so the names with commas + will be rejected. This applies only to names of the TXT record and not values + since values (i.e.<code>value</code> contents) supports multiple values + separated by commas. +<span class="since">Since 0.9.1</span> </dd> </dl>

See my above comment. Note that some of what you say here doesn't apply if you move the <dns> record up to the top level of the hierarchy in <network>.

diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 6d01b06..e27dace 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -136,6 +136,18 @@ </optional> </element> </optional> +<optional> +<!-- Define the DNS related elements like TXT records + and other features --> +<element name="dns"> +<zeroOrMore> +<element name="txt-record">

I prefer simply "txt" instead of "txt-record", since 1) it avoids having a - in the name, 2) that's what it's called in a DNS zone file, 3) the "-record" is really an artifact of the dnsmasq implementation (note that dnsmasq inconsistently uses "srv-host" and "cname", along with "ptr-record" and "naptr-record" - these are *all* "records" in the DNS zone file.

+<attribute name="name"><text/></attribute> +<attribute name="value"><text/></attribute> +</element> +</zeroOrMore> +</element> +</optional>

This will move up one level. I won't bother pointing out the other places where code will change due to moving <dns> up in the hierarchy...

</element> </zeroOrMore> </interleave> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index dcab9de..b7427d0 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -435,6 +435,60 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, }

static int +virNetworkDNSDefParseXML(virNetworkIpDefPtr def, + xmlNodePtr node) +{ + + xmlNodePtr cur; + int result = -1; + + if (VIR_ALLOC(def->dns)) + goto oom_error; + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE&& + xmlStrEqual(cur->name, BAD_CAST "txt-record")) { + char *name, *value; + + if (!(name = virXMLPropString(cur, "name"))) { + cur = cur->next; + continue; + } + if (!(value = virXMLPropString(cur, "value"))) { + VIR_FREE(name); + cur = cur->next; + continue; + } + + if (strchr(name, ' ') != NULL) { + virNetworkReportError(VIR_ERR_XML_DETAIL, + _("TXT record names in DNS doesn't support spaces"));

s/doesn't/don't/. Also, an error message is much more helpful if it provides context - adding the name and value to the message would be very useful.

+ return -1;

You leaked both name and value here.

+ } + + if (VIR_REALLOC_N(def->dns->txtrecords, def->dns->ntxtrecords + 1)< 0) + goto oom_error;

You again leaked name and value. (def->dns->txtrecords and def->dns will be cleaned up by the caller as it destructs the def).

+ def->dns->txtrecords[def->dns->ntxtrecords].name = strdup(name); + def->dns->txtrecords[def->dns->ntxtrecords].value = strdup(value); + def->dns->ntxtrecords++; + + VIR_FREE(name); + VIR_FREE(value);

Instead of doing a strdup() of each of these, immediately followed by freeing the original, you should just assign the original string into the def.

+ } + + cur = cur->next; + } + + return 0; + +oom_error: + virReportOOMError(); + return result;

I actually prefer having a single exit, with virReportOOMError() called at the location of the failure, and your "return 0" turned into "ret = 0". You could even do a VIR_FREE(name); VIR_FREE(value); here (after setting them to NULL when you move the pointer into the txtrecord array). That way new error path code will never forget to free them, and as a bonus the variable "ret" is actually used for something.

+} + +static int virNetworkIPParseXML(const char *networkName, virNetworkIpDefPtr def, xmlNodePtr node, @@ -550,6 +604,12 @@ virNetworkIPParseXML(const char *networkName, goto error;

} else if (cur->type == XML_ELEMENT_NODE&& + xmlStrEqual(cur->name, BAD_CAST "dns")) { + result = virNetworkDNSDefParseXML(def, cur); + if (result) + goto error; +

Okay, I can't stop myself :-) - this should be up a level, in virNetworkParseXML().

+ } else if (cur->type == XML_ELEMENT_NODE&& xmlStrEqual(cur->name, BAD_CAST "tftp")) { char *root;

@@ -828,6 +888,17 @@ virNetworkIpDefFormat(virBufferPtr buf,

virBufferAddLit(buf, "</dhcp>\n"); } + if ((def->dns != NULL)&& (def->dns->ntxtrecords)) { + int ii; + + virBufferAddLit(buf, "<dns>\n"); + for (ii = 0 ; ii< def->dns->ntxtrecords ; ii++) { + virBufferVSprintf(buf, "<txt-record name='%s' value='%s' />\n", + def->dns->txtrecords[ii].name, + def->dns->txtrecords[ii].value); + } + virBufferAddLit(buf, "</dns>\n"); + }

same.

virBufferAddLit(buf, "</ip>\n");

diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 281124b..5f47595 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -57,6 +57,20 @@ struct _virNetworkDHCPHostDef { virSocketAddr ip; };

+typedef struct _virNetworkDNSTxtRecordsDef virNetworkDNSTxtRecordsDef; +typedef virNetworkDNSTxtRecordsDef *virNetworkDNSTxtRecordsDefPtr; +struct _virNetworkDNSTxtRecordsDef { + char *name; + char *value; +}; + +struct virNetworkDNSDef { + unsigned int ntxtrecords; + virNetworkDNSTxtRecordsDefPtr txtrecords; +} virNetworkDNSDef; + +typedef struct virNetworkDNSDef *virNetworkDNSDefPtr; + typedef struct _virNetworkIpDef virNetworkIpDef; typedef virNetworkIpDef *virNetworkIpDefPtr; struct _virNetworkIpDef { @@ -75,6 +89,8 @@ struct _virNetworkIpDef { unsigned int nranges; /* Zero or more dhcp ranges */ virNetworkDHCPRangeDefPtr ranges;

+ virNetworkDNSDefPtr dns; /* DNS related settings for DNSMasq */ +

Move this into virNetworkDef (and no need to mention dnsmasq here, as this file could be used by other implementations in the future).

unsigned int nhosts; /* Zero or more dhcp hosts */ virNetworkDHCPHostDefPtr hosts;

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index ea2bfd4..2e299f5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -442,7 +442,6 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef, return 0; }

- static int networkBuildDnsmasqArgv(virNetworkObjPtr network, virNetworkIpDefPtr ipdef, @@ -497,6 +496,20 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) virCommandAddArg(cmd, "--dhcp-option=3");

+ if (ipdef->dns != NULL) { + int i; + + for (i = 0; i< ipdef->dns->ntxtrecords; i++) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferVSprintf(&buf, "%s,%s", + ipdef->dns->txtrecords[i].name, + ipdef->dns->txtrecords[i].value); + + virCommandAddArgPair(cmd, "--txt-record", virBufferContentAndReset(&buf)); + VIR_FREE(buf); + } + } + /* * --interface does not actually work with dnsmasq< 2.47, * due to DAD for ipv6 addresses on the interface. diff --git a/tests/networkxml2xmlin/nat-network-dns-txt-record.xml b/tests/networkxml2xmlin/nat-network-dns-txt-record.xml new file mode 100644 index 0000000..d3e795d --- /dev/null +++ b/tests/networkxml2xmlin/nat-network-dns-txt-record.xml @@ -0,0 +1,24 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> +<host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> +</dhcp> +<dns> +<txt-record name='example' value='example value' /> +</dns> +</ip> +<ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> +</ip> +<ip family='ipv4' address='10.24.10.1'> +</ip> +</network> diff --git a/tests/networkxml2xmlout/nat-network-dns-txt-record.xml b/tests/networkxml2xmlout/nat-network-dns-txt-record.xml new file mode 100644 index 0000000..d3e795d --- /dev/null +++ b/tests/networkxml2xmlout/nat-network-dns-txt-record.xml @@ -0,0 +1,24 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> +<host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> +</dhcp> +<dns> +<txt-record name='example' value='example value' /> +</dns> +</ip> +<ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> +</ip> +<ip family='ipv4' address='10.24.10.1'> +</ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 7805548..beb00ef 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -90,6 +90,7 @@ mymain(int argc, char **argv) DO_TEST("nat-network"); DO_TEST("netboot-network"); DO_TEST("netboot-proxy-network"); + DO_TEST("nat-network-dns-txt-record");

return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); }

On 04/01/2011 06:45 AM, Michal Novotny wrote:

Make: passed Make check: passed Make syntax-check: passed

Hi, this is the patch that is adding regression tests for the network bridge driver command-line arguments similar way it is done for QEMU driver. This is checking the built dnsmasq parameters (using networkBuildDhcpDaemonCommandLine() function) and comparing them to excepted arguments in the *.argv files.

This has been tested and working fine.

Michal

Same comments about the commit message as in 1/5 - don't include stuff about what tests passed, salutations, signatures; *do* include a short sample of the XML.

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- src/network/bridge_driver.c | 27 ++++- src/network/bridge_driver.h | 3 + tests/Makefile.am | 11 ++ tests/networkxml2argvdata/isolated-network.argv | 1 + tests/networkxml2argvdata/isolated-network.xml | 11 ++ .../nat-network-dns-txt-record.argv | 1 + .../nat-network-dns-txt-record.xml | 24 ++++ tests/networkxml2argvdata/nat-network.argv | 1 + tests/networkxml2argvdata/nat-network.xml | 21 ++++ tests/networkxml2argvdata/netboot-network.argv | 1 + tests/networkxml2argvdata/netboot-network.xml | 14 +++ .../networkxml2argvdata/netboot-proxy-network.argv | 1 + .../networkxml2argvdata/netboot-proxy-network.xml | 13 ++ tests/networkxml2argvdata/routed-network.argv | 1 + tests/networkxml2argvdata/routed-network.xml | 9 ++ tests/networkxml2argvtest.c | 119 ++++++++++++++++++++ 16 files changed, 255 insertions(+), 3 deletions(-) create mode 100644 tests/networkxml2argvdata/isolated-network.argv create mode 100644 tests/networkxml2argvdata/isolated-network.xml create mode 100644 tests/networkxml2argvdata/nat-network-dns-txt-record.argv create mode 100644 tests/networkxml2argvdata/nat-network-dns-txt-record.xml create mode 100644 tests/networkxml2argvdata/nat-network.argv create mode 100644 tests/networkxml2argvdata/nat-network.xml create mode 100644 tests/networkxml2argvdata/netboot-network.argv create mode 100644 tests/networkxml2argvdata/netboot-network.xml create mode 100644 tests/networkxml2argvdata/netboot-proxy-network.argv create mode 100644 tests/networkxml2argvdata/netboot-proxy-network.xml create mode 100644 tests/networkxml2argvdata/routed-network.argv create mode 100644 tests/networkxml2argvdata/routed-network.xml create mode 100644 tests/networkxml2argvtest.c

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2e299f5..b6ce39d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -613,11 +613,11 @@ cleanup: return ret; }

-static int -networkStartDhcpDaemon(virNetworkObjPtr network) +int +networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile)

If you make a function global, you should add it to a .syms file. In this case (as we just discussed on IRC with eblake) you should create a new src/libvirt_network.syms file and add it to that (then add that .syms file to the appropriate places in src/Makefile.am)

{ virCommandPtr cmd = NULL; - char *pidfile = NULL;

As patched, this will not compile - you removed pidfile from the new function, but left the assignment to it in. (actually, all of the directory and file creation items should be moved down into networkStartDhcpDaemon, so that networkBuildDhcpDaemonCommandLine doesn't have any side effects.)

int ret = -1, err, ii; virNetworkIpDefPtr ipdef;

@@ -666,6 +666,27 @@ networkStartDhcpDaemon(virNetworkObjPtr network) goto cleanup; }

+ if (cmdout) + *cmdout = cmd; + + ret = 0; +cleanup: + if (ret != 0)

The standard practice in libvirt is to use "ret < 0" rather than "ret != 0".

+ virCommandFree(cmd); + return ret; +} + +static int +networkStartDhcpDaemon(virNetworkObjPtr network) +{ + virCommandPtr cmd = NULL; + char *pidfile = NULL; + int ret = -1; + + ret = networkBuildDhcpDaemonCommandLine(network,&cmd, pidfile); + if (ret != 0)

Again, ret < 0.

+ goto cleanup; + if (virCommandRun(cmd, NULL)< 0) goto cleanup;

diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 32d2ae7..8d82b67 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -28,7 +28,10 @@ # include<config.h>

# include "internal.h" +# include "network_conf.h" +# include "command.h"

int networkRegister(void); +int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile);

#endif /* __VIR_NETWORK__DRIVER_H */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 5896442..a3f8d00 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -50,6 +50,7 @@ EXTRA_DIST = \ networkschematest \ networkxml2xmlin \ networkxml2xmlout \ + networkxml2argvdata \

Yay for new tests!

nodedevschemadata \ nodedevschematest \ nodeinfodata \ @@ -104,6 +105,8 @@ endif

check_PROGRAMS += networkxml2xmltest

+check_PROGRAMS += networkxml2argvtest + check_PROGRAMS += nwfilterxml2xmltest

check_PROGRAMS += storagevolxml2xmltest storagepoolxml2xmltest @@ -191,6 +194,8 @@ endif

TESTS += networkxml2xmltest

+TESTS += networkxml2argvtest + TESTS += storagevolxml2xmltest storagepoolxml2xmltest

TESTS += nodedevxml2xmltest @@ -308,6 +313,12 @@ networkxml2xmltest_SOURCES = \ testutils.c testutils.h networkxml2xmltest_LDADD = $(LDADDS)

+networkxml2argvtest_SOURCES = \ + networkxml2argvtest.c \ + ../src/network/bridge_driver.c network/bridge_driver.h \

Rather than adding the source files, you should be adding the .la file libvirt_network.la. See other .la file additions for the proper pattern to follow.

+ testutils.c testutils.h +networkxml2argvtest_LDADD = $(LDADDS) + nwfilterxml2xmltest_SOURCES = \ nwfilterxml2xmltest.c \ testutils.c testutils.h diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv new file mode 100644 index 0000000..1c173db --- /dev/null +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/private.pid --conf-file= --except-interface lo --dhcp-option=3 --listen-address 192.168.152.1 --dhcp-range 192.168.152.2,192.168.152.254 --dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases --dhcp-lease-max=253 --dhcp-no-override diff --git a/tests/networkxml2argvdata/isolated-network.xml b/tests/networkxml2argvdata/isolated-network.xml new file mode 100644 index 0000000..cc320a9 --- /dev/null +++ b/tests/networkxml2argvdata/isolated-network.xml @@ -0,0 +1,11 @@ +<network> +<name>private</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<bridge name='virbr2' stp='on' delay='0' /> +<mac address='52:54:00:17:3F:37'/> +<ip address='192.168.152.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.152.2' end='192.168.152.254' /> +</dhcp> +</ip> +</network> diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv new file mode 100644 index 0000000..55dcf02 --- /dev/null +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/default.pid --conf-file= --except-interface lo --txt-record=example,example value --listen-address 192.168.122.1

Does the argument to ==txt-record need to be quoted? (probably not needed, but it might help readability in the logs - will it *hurt* anything to quote it?

--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 --dhcp-range 192.168.122.2,192.168.122.254 --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases --dhcp-lease-max=253 --dhcp-no-override --dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.xml b/tests/networkxml2argvdata/nat-network-dns-txt-record.xml new file mode 100644 index 0000000..d3e795d --- /dev/null +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.xml @@ -0,0 +1,24 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> +<host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> +</dhcp> +<dns> +<txt-record name='example' value='example value' /> +</dns> +</ip> +<ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> +</ip> +<ip family='ipv4' address='10.24.10.1'> +</ip> +</network> diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv new file mode 100644 index 0000000..95ee6d9 --- /dev/null +++ b/tests/networkxml2argvdata/nat-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/default.pid --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 --dhcp-range 192.168.122.2,192.168.122.254 --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases --dhcp-lease-max=253 --dhcp-no-override --dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile diff --git a/tests/networkxml2argvdata/nat-network.xml b/tests/networkxml2argvdata/nat-network.xml new file mode 100644 index 0000000..eb71d9e --- /dev/null +++ b/tests/networkxml2argvdata/nat-network.xml @@ -0,0 +1,21 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> +<host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> +</dhcp> +</ip> +<ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> +</ip> +<ip family='ipv4' address='10.24.10.1'> +</ip> +</network> diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv new file mode 100644 index 0000000..36c2360 --- /dev/null +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --domain example.com --pid-file=/var/run/libvirt/network/netboot.pid --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 --enable-tftp --tftp-root /var/lib/tftproot --dhcp-boot pxeboot.img diff --git a/tests/networkxml2argvdata/netboot-network.xml b/tests/networkxml2argvdata/netboot-network.xml new file mode 100644 index 0000000..b8a4d99 --- /dev/null +++ b/tests/networkxml2argvdata/netboot-network.xml @@ -0,0 +1,14 @@ +<network> +<name>netboot</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward mode='nat'/> +<bridge name='virbr1' stp='off' delay='1' /> +<domain name='example.com'/> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<tftp root='/var/lib/tftproot' /> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<bootp file='pxeboot.img' /> +</dhcp> +</ip> +</network> diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv new file mode 100644 index 0000000..da97b72 --- /dev/null +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --domain example.com --pid-file=/var/run/libvirt/network/netboot.pid --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 --dhcp-boot pxeboot.img,,10.20.30.40 diff --git a/tests/networkxml2argvdata/netboot-proxy-network.xml b/tests/networkxml2argvdata/netboot-proxy-network.xml new file mode 100644 index 0000000..e11c50b --- /dev/null +++ b/tests/networkxml2argvdata/netboot-proxy-network.xml @@ -0,0 +1,13 @@ +<network> +<name>netboot</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward mode='nat'/> +<bridge name='virbr1' stp='off' delay='1' /> +<domain name='example.com'/> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<bootp file='pxeboot.img' server='10.20.30.40' /> +</dhcp> +</ip> +</network> diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv new file mode 100644 index 0000000..443087c --- /dev/null +++ b/tests/networkxml2argvdata/routed-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/local.pid --conf-file= --except-interface lo --listen-address 192.168.122.1 diff --git a/tests/networkxml2argvdata/routed-network.xml b/tests/networkxml2argvdata/routed-network.xml new file mode 100644 index 0000000..3aa8109 --- /dev/null +++ b/tests/networkxml2argvdata/routed-network.xml @@ -0,0 +1,9 @@ +<network> +<name>local</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='route'/> +<bridge name='virbr1' stp='on' delay='0' /> +<mac address='12:34:56:78:9A:BC'/> +<ip address='192.168.122.1' netmask='255.255.255.0'> +</ip> +</network> diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c new file mode 100644 index 0000000..e3a8bb4 --- /dev/null +++ b/tests/networkxml2argvtest.c @@ -0,0 +1,119 @@ +#include<config.h> + +#include<stdio.h> +#include<stdlib.h> +#include<unistd.h> +#include<string.h> + +#include<sys/types.h> +#include<fcntl.h> + +#include "internal.h" +#include "testutils.h" +#include "network_conf.h" +#include "command.h" +#include "memory.h" +#include "network/bridge_driver.h" + +static char *progname; +static char *abs_srcdir; + +#define MAX_FILE 4096 + + +static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { + char inXmlData[MAX_FILE]; + char *inXmlPtr =&(inXmlData[0]); + char outArgvData[MAX_FILE]; + char *outArgvPtr =&(outArgvData[0]); + char *actual = NULL; + int ret = -1; + virNetworkDefPtr dev = NULL; + virNetworkObjPtr obj = NULL; + virCommandPtr cmd = NULL; + char *pidfile = NULL; + + if (virtTestLoadFile(inxml,&inXmlPtr, MAX_FILE)< 0) + goto fail; + + if (virtTestLoadFile(outargv,&outArgvPtr, MAX_FILE)< 0) + goto fail; + + if (!(dev = virNetworkDefParseString(inXmlData))) + goto fail; + + if (VIR_ALLOC(obj)< 0) + goto fail; + + obj->def = dev; + + if (networkBuildDhcpDaemonCommandLine(obj,&cmd, pidfile) != 0) + goto fail; + + if (!(actual = virCommandToString(cmd))) + goto fail; + + /* There is a new line character but syntax-check would complain + * about this in argv files so just trim it now + */ + outArgvData[ strlen(outArgvData) - 1] = 0; + + if (STRNEQ(outArgvData, actual)) { + virtTestDifference(stderr, outArgvData, actual); + goto fail; + } + + ret = 0; + + fail: + free(actual); + VIR_FREE(pidfile); + virCommandFree(cmd); + virNetworkObjFree(obj); + return ret; +} + +static int testCompareXMLToArgvHelper(const void *data) { + char inxml[PATH_MAX]; + char outargv[PATH_MAX]; + snprintf(inxml, PATH_MAX, "%s/networkxml2argvdata/%s.xml", + abs_srcdir, (const char*)data); + snprintf(outargv, PATH_MAX, "%s/networkxml2argvdata/%s.argv", + abs_srcdir, (const char*)data); + return testCompareXMLToArgvFiles(inxml, outargv); +} + + +static int +mymain(int argc, char **argv) +{ + int ret = 0; + char cwd[PATH_MAX]; + + progname = argv[0]; + + if (argc> 1) { + fprintf(stderr, "Usage: %s\n", progname); + return (EXIT_FAILURE); + } + + abs_srcdir = getenv("abs_srcdir"); + if (!abs_srcdir) + abs_srcdir = getcwd(cwd, sizeof(cwd)); + +#define DO_TEST(name) \ + if (virtTestRun("Network XML-2-Argv " name, \ + 1, testCompareXMLToArgvHelper, (name))< 0) \ + ret = -1 + + DO_TEST("isolated-network"); + DO_TEST("routed-network"); + DO_TEST("nat-network"); + DO_TEST("netboot-network"); + DO_TEST("netboot-proxy-network"); + DO_TEST("nat-network-dns-txt-record"); + + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain)

Coming back to this now that I see how it's being used... On 04/01/2011 06:45 AM, Michal Novotny wrote:

Hi, this is the patch to introduce the internal infrastructure for additional hosts for network bridge driver using the addnhosts* API functions.

This is necessary for next part of the patch to support DNS hosts definition in the network XML description.

Michal

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 3 + src/util/dnsmasq.c | 266 ++++++++++++++++++++++++++++++++++++++++++- src/util/dnsmasq.h | 22 ++++- 4 files changed, 287 insertions(+), 5 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 65a86d3..73c3f77 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -186,6 +186,7 @@ virUnrefStream;

# dnsmasq.h dnsmasqAddDhcpHost; +dnsmasqAddHost; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 41b14f9..4ad3143 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -589,6 +589,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, if (dctx->hostsfile->nhosts) virCommandAddArgPair(cmd, "--dhcp-hostsfile", dctx->hostsfile->path); + if (dctx->addnhostsfile->nhosts) + virCommandAddArgPair(cmd, "--addn-hosts", + dctx->addnhostsfile->path);

dnsmasqContextFree(dctx); } diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c index be230e1..fee3b90 100644 --- a/src/util/dnsmasq.c +++ b/src/util/dnsmasq.c @@ -48,6 +48,7 @@

#define VIR_FROM_THIS VIR_FROM_NETWORK #define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" +#define DNSMASQ_ADDNHOSTSFILE_SUFFIX "addnhosts"

static void dhcphostFree(dnsmasqDhcpHost *host) @@ -56,6 +57,231 @@ dhcphostFree(dnsmasqDhcpHost *host) }

static void +addnhostFree(dnsmasqAddnHost *host) +{ + VIR_FREE(host->hostnames);

You haven't freed host->hostnames[0], hostnames[1], etc; you've only freed the array of pointers.

+ VIR_FREE(host->ip); +} + +static void +addnhostsFree(dnsmasqAddnHostsfile *addnhostsfile) +{ + unsigned int i; + + if (addnhostsfile->hosts) { + for (i = 0; i< addnhostsfile->nhosts; i++) + addnhostFree(&addnhostsfile->hosts[i]); + + VIR_FREE(addnhostsfile->hosts); + + addnhostsfile->nhosts = 0; + } + + VIR_FREE(addnhostsfile->path); + + VIR_FREE(addnhostsfile); +} + +static int +addnhostsAdd(dnsmasqAddnHostsfile *addnhostsfile, + virSocketAddr *ip,

You could also mark ip as a const.

+ const char *name) +{

It looks like this function could be simplified quite a lot if it was changed to take a char** instead of a char* (see my comment in 5/5 about saving it that way in the virNetworkDNSDefPtr).

+ char *ipstr = NULL; + int idx = -1; + int i; + + if (!(ipstr = virSocketFormatAddr(ip))) + return -1; + + for (i = 0; i< addnhostsfile->nhosts; i++) { + if (STREQ((const char *)addnhostsfile->hosts[i].ip, (const char *)ipstr)) { + idx = i; + break; + } + } + + if (idx< 0) { + if (VIR_REALLOC_N(addnhostsfile->hosts, addnhostsfile->nhosts + 1)< 0) + goto alloc_error; + + idx = addnhostsfile->nhosts; + if (VIR_ALLOC(addnhostsfile->hosts[idx].hostnames)< 0) + goto alloc_error; + + if (virAsprintf(&addnhostsfile->hosts[idx].ip, "%s", ipstr)< 0) + goto alloc_error; + + addnhostsfile->hosts[idx].nhostnames = 0; + addnhostsfile->nhosts++; + } + + if (VIR_REALLOC_N(addnhostsfile->hosts[idx].hostnames, addnhostsfile->hosts[idx].nhostnames + 1)< 0) + goto alloc_error; + + if (virAsprintf(&addnhostsfile->hosts[idx].hostnames[addnhostsfile->hosts[idx].nhostnames], "%s", name)< 0) + goto alloc_error; + + VIR_FREE(ipstr); + + addnhostsfile->hosts[idx].nhostnames++; + + return 0; + + alloc_error: + virReportOOMError(); + VIR_FREE(ipstr); + return -1; +} + +static dnsmasqAddnHostsfile * +addnhostsNew(const char *name, + const char *config_dir) +{ + int err; + dnsmasqAddnHostsfile *addnhostsfile; + + if (VIR_ALLOC(addnhostsfile)< 0) { + virReportOOMError(); + return NULL; + } + + addnhostsfile->hosts = NULL; + addnhostsfile->nhosts = 0; + + if (virAsprintf(&addnhostsfile->path, "%s/%s.%s", config_dir, name, + DNSMASQ_ADDNHOSTSFILE_SUFFIX)< 0) { + virReportOOMError(); + goto error; + } + + if ((err = virFileMakePath(config_dir))) { + virReportSystemError(err, _("cannot create config directory '%s'"), + config_dir); + goto error; + } + + return addnhostsfile; + + error: + addnhostsFree(addnhostsfile); + return NULL; +} + +static int +addnhostsWrite(const char *path, + dnsmasqAddnHost *hosts, + unsigned int nhosts) +{ + char *tmp; + FILE *f; + bool istmp = true; + unsigned int i, ii; + int rc = 0; + + if (nhosts == 0) + return rc; + + if (virAsprintf(&tmp, "%s.new", path)< 0) + return ENOMEM; + + if (!(f = fopen(tmp, "w"))) { + istmp = false; + if (!(f = fopen(path, "w"))) { + rc = errno; + goto cleanup; + } + } + + for (i = 0; i< nhosts; i++) { + if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { + rc = errno; + VIR_FORCE_FCLOSE(f); + + if (istmp) + unlink(tmp); + + goto cleanup; + } + + for (ii = 0; ii< hosts[i].nhostnames; ii++) { + if (fputs(hosts[i].hostnames[ii], f) == EOF || fputc('\t', f) == EOF) { + rc = errno; + VIR_FORCE_FCLOSE(f); + + if (istmp) + unlink(tmp); + + goto cleanup; + } + } + + if (fputc('\n', f) == EOF) { + rc = errno; + VIR_FORCE_FCLOSE(f); + + if (istmp) + unlink(tmp); + + goto cleanup; + } + } + + if (VIR_FCLOSE(f) == EOF) { + rc = errno; + goto cleanup; + } + + if (istmp) { + if (rename(tmp, path)< 0) { + rc = errno; + unlink(tmp); + goto cleanup; + } + + if (unlink(tmp)< 0) { + rc = errno; + goto cleanup; + } + } + + cleanup: + VIR_FREE(tmp); + + return rc; +} + +static int +addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile) +{ + int err = addnhostsWrite(addnhostsfile->path, addnhostsfile->hosts, + addnhostsfile->nhosts); + + if (err< 0) { + virReportSystemError(err, _("cannot write config file '%s'"), + addnhostsfile->path); + return -1; + } + + return 0; +} + +static int +addnhostsDelete(dnsmasqAddnHostsfile *addnhostsfile) +{ + if (!virFileExists(addnhostsfile->path)) + return 0; + + if (unlink(addnhostsfile->path)< 0) { + virReportSystemError(errno, _("cannot remove config file '%s'"), + addnhostsfile->path); + return -1; + } + + return 0; +} + +static void hostsfileFree(dnsmasqHostsfile *hostsfile) { unsigned int i; @@ -255,6 +481,8 @@ dnsmasqContextNew(const char *network_name,

if (!(ctx->hostsfile = hostsfileNew(network_name, config_dir))) goto error; + if (!(ctx->addnhostsfile = addnhostsNew(network_name, config_dir))) + goto error;

return ctx;

@@ -277,6 +505,8 @@ dnsmasqContextFree(dnsmasqContext *ctx)

if (ctx->hostsfile) hostsfileFree(ctx->hostsfile); + if (ctx->addnhostsfile) + addnhostsFree(ctx->addnhostsfile);

VIR_FREE(ctx); } @@ -300,6 +530,24 @@ dnsmasqAddDhcpHost(dnsmasqContext *ctx, hostsfileAdd(ctx->hostsfile, mac, ip, name); }

+/* + * dnsmasqAddHost: + * @ctx: pointer to the dnsmasq context for each network + * @ip: pointer to the socket address contains ip of the host + * @name: pointer to the string contains hostname of the host + * + * Add additional host entry. + */ + +void +dnsmasqAddHost(dnsmasqContext *ctx, + virSocketAddr *ip, + const char *name) +{ + if (ctx->addnhostsfile) + addnhostsAdd(ctx->addnhostsfile, ip, name); +} + /** * dnsmasqSave: * @ctx: pointer to the dnsmasq context for each network @@ -309,10 +557,15 @@ dnsmasqAddDhcpHost(dnsmasqContext *ctx, int dnsmasqSave(const dnsmasqContext *ctx) { + int ret1 = 0; + int ret2 = 0; + if (ctx->hostsfile) - return hostsfileSave(ctx->hostsfile); + ret1 = hostsfileSave(ctx->hostsfile); + if (ctx->addnhostsfile) + ret2 = addnhostsSave(ctx->addnhostsfile);

- return 0; + return ((ret1 == 0)&& (ret2 == 0)) ? 0 : -1; }

@@ -325,10 +578,15 @@ dnsmasqSave(const dnsmasqContext *ctx) int dnsmasqDelete(const dnsmasqContext *ctx) { + int ret1 = 0; + int ret2 = 0; + if (ctx->hostsfile) - return hostsfileDelete(ctx->hostsfile); + ret1 = hostsfileDelete(ctx->hostsfile); + if (ctx->addnhostsfile) + ret2 = addnhostsDelete(ctx->addnhostsfile);

- return 0; + return ((ret1 == 0)&& (ret2 == 0)) ? 0 : -1; }

/** diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h index 02a961f..3f6320a 100644 --- a/src/util/dnsmasq.h +++ b/src/util/dnsmasq.h @@ -44,7 +44,24 @@ typedef struct

typedef struct { - dnsmasqHostsfile *hostsfile; + unsigned int nhostnames; + char *ip; + char **hostnames; + +} dnsmasqAddnHost; + +typedef struct +{ + unsigned int nhosts; + dnsmasqAddnHost *hosts; + + char *path; /* Absolute path of dnsmasq's hostsfile. */ +} dnsmasqAddnHostsfile; + +typedef struct +{ + dnsmasqHostsfile *hostsfile; + dnsmasqAddnHostsfile *addnhostsfile; } dnsmasqContext;

dnsmasqContext * dnsmasqContextNew(const char *network_name, @@ -54,6 +71,9 @@ void dnsmasqAddDhcpHost(dnsmasqContext *ctx, const char *mac, virSocketAddr *ip, const char *name); +void dnsmasqAddHost(dnsmasqContext *ctx, + virSocketAddr *ip, + const char *name); int dnsmasqSave(const dnsmasqContext *ctx); int dnsmasqDelete(const dnsmasqContext *ctx); int dnsmasqReload(pid_t pid);

On 04/01/2011 06:45 AM, Michal Novotny wrote:

Make: passed Make check: passed Make syntax-check: passed

Hi, this is the patch to add support for DNS hosts definition in the network XML description to generate the the hosts file. This patch uses the addnhosts* APIs implemented to the src/util/dnsmasq.c by part 2 of this patch series.

Also, tests for the XML to XML definition and command-line regression tests has been added.

Michal

Same comments about the commit message - remove the testing comments, salutation, signature; add in a short example of the XML.

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- docs/formatnetwork.html.in | 7 + docs/schemas/network.rng | 8 ++ src/conf/network_conf.c | 128 ++++++++++++++++++-- src/conf/network_conf.h | 9 ++ src/network/bridge_driver.c | 20 ++- .../networkxml2argvdata/nat-network-dns-hosts.argv | 1 + .../networkxml2argvdata/nat-network-dns-hosts.xml | 19 +++ tests/networkxml2argvtest.c | 1 + tests/networkxml2xmlin/nat-network-dns-hosts.xml | 27 ++++ tests/networkxml2xmlout/nat-network-dns-hosts.xml | 27 ++++ tests/networkxml2xmltest.c | 1 + 11 files changed, 234 insertions(+), 14 deletions(-) create mode 100644 tests/networkxml2argvdata/nat-network-dns-hosts.argv create mode 100644 tests/networkxml2argvdata/nat-network-dns-hosts.xml create mode 100644 tests/networkxml2xmlin/nat-network-dns-hosts.xml create mode 100644 tests/networkxml2xmlout/nat-network-dns-hosts.xml

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 5211ed2..5d18ed9 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -222,6 +222,13 @@ separated by commas. <span class="since">Since 0.9.1</span> </dd> +<dt><code>host</code></dt> +<dd>The<code>host</code> element is the definition of DNS hosts to be passed + to the DNS service. The IP address is identified by the<code>ip</code> attribute + and the names for the IP addresses are identified in the<code>hostname</code> + subelements of the<code>host</code> element. +<span class="since">Since 0.9.1</span> +</dd> </dl>

<h2><a name="examples">Example configuration</a></h2> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index e27dace..05066a5 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -146,6 +146,14 @@ <attribute name="value"><text/></attribute> </element> </zeroOrMore> +<zeroOrMore> +<element name="host"> +<attribute name="ip"><ref name="ipv4-addr"/></attribute> +<oneOrMore> +<element name="hostname"><text/></element>

I think it would be better to simply call it "name" rather than "hostname" (since "host" is already implied by its parent being "<host>")

+</oneOrMore> +</element> +</zeroOrMore>

Also, this will move up a level along with the rest of <dns>.

</element> </optional> </element> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b7427d0..1f88649 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -435,6 +435,53 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, }

static int +virNetworkDNSHostsDefParseXML(virNetworkIpDefPtr def,

(will be virNetworkDefPtr def...)

+ xmlNodePtr node, + virSocketAddr ip) +{ + xmlNodePtr cur; + int result = -1; + + if (def->dns->hosts == NULL) { + if (VIR_ALLOC(def->dns->hosts)< 0) + goto oom_error; + def->dns->nhosts = 0; + } + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE&& + xmlStrEqual(cur->name, BAD_CAST "hostname")) {

Again, s/hostname/name/

+ if (cur->children != NULL) { + char *hostname; + + hostname = strdup((char *)cur->children->content); + + if (VIR_REALLOC_N(def->dns->hosts, def->dns->nhosts + 1)< 0) { + VIR_FREE(hostname); + result = -1; + goto oom_error; + } + + def->dns->hosts[def->dns->nhosts].name = strdup(hostname);

Rather than strduping hostname, then freeing it, just assign the original directly into the struct.

+ def->dns->hosts[def->dns->nhosts].ip = ip; + def->dns->nhosts++; + + VIR_FREE(hostname); + } + } + + cur = cur->next; + } + + return 0; + +oom_error: + virReportOOMError(); + return result; +} + +static int virNetworkDNSDefParseXML(virNetworkIpDefPtr def, xmlNodePtr node) { @@ -476,6 +523,27 @@ virNetworkDNSDefParseXML(virNetworkIpDefPtr def,

VIR_FREE(name); VIR_FREE(value); + } else if (cur->type == XML_ELEMENT_NODE&& + xmlStrEqual(cur->name, BAD_CAST "host")) { + char *ip; + virSocketAddr inaddr; + memset(&inaddr, 0, sizeof(inaddr)); + + if (!(ip = virXMLPropString(cur, "ip"))) { + cur = cur->next; + continue; + } + if ((ip == NULL) || + (virSocketParseAddr(ip,&inaddr, AF_UNSPEC)< 0)) { + virNetworkReportError(VIR_ERR_XML_DETAIL, + _("Missing IP address in DNS host definition"));

"Missing/incorrect". Also, since def will end up being virNetworkDefPtr, you can add the name of the network, as well as the string that was given for ip. That will help in finding the source of the error.

+ VIR_FREE(ip); + goto error; + } + VIR_FREE(ip); + result = virNetworkDNSHostsDefParseXML(def, cur, inaddr); + if (result) + goto error; }

cur = cur->next; @@ -485,6 +553,7 @@ virNetworkDNSDefParseXML(virNetworkIpDefPtr def,

oom_error: virReportOOMError(); +error: return result; }

@@ -888,17 +957,60 @@ virNetworkIpDefFormat(virBufferPtr buf,

virBufferAddLit(buf, "</dhcp>\n"); } - if ((def->dns != NULL)&& (def->dns->ntxtrecords)) { - int ii; - + if (def->dns != NULL) { virBufferAddLit(buf, "<dns>\n"); - for (ii = 0 ; ii< def->dns->ntxtrecords ; ii++) { - virBufferVSprintf(buf, "<txt-record name='%s' value='%s' />\n", - def->dns->txtrecords[ii].name, - def->dns->txtrecords[ii].value); + + if (def->dns->ntxtrecords) {

This if() should have been in the patch for <txt>. That way this patch wouldn't be polluted with diffs unrelated to the new feature introduced in this patch.

+ int ii; + + for (ii = 0 ; ii< def->dns->ntxtrecords; ii++) { + virBufferVSprintf(buf, "<txt-record name='%s' value='%s' />\n", + def->dns->txtrecords[ii].name, + def->dns->txtrecords[ii].value); + } + } + if (def->dns->nhosts) { + int ii, j; + char **iplist = NULL; + int iplist_size = 0; + bool in_list; + + if (VIR_ALLOC(iplist)< 0) + goto error; + + for (ii = 0 ; ii< def->dns->nhosts; ii++) { + char *ip = virSocketFormatAddr(&def->dns->hosts[ii].ip); + in_list = false; + for (j = 0; j< iplist_size; j++) + if (STREQ(iplist[j], ip)) + in_list = true; + + if (!in_list) { + virBufferVSprintf(buf, "<host ip='%s'>\n", ip); + + for (j = 0 ; j< def->dns->nhosts; j++) { + char *thisip = virSocketFormatAddr(&def->dns->hosts[j].ip); + if (STREQ(ip, thisip)) + virBufferVSprintf(buf, "<hostname>%s</hostname>\n", + def->dns->hosts[j].name); + } + virBufferVSprintf(buf, "</host>\n"); + + if (VIR_REALLOC_N(iplist, iplist_size + 1)< 0) + goto error; + + iplist[iplist_size] = strdup(ip); + iplist_size++; + } + } + + for (j = 0; j< iplist_size; j++) + VIR_FREE(iplist[j]); + VIR_FREE(iplist);

It would make more sense if virNetworkDNSHostsDef->name was char** and had an array of names, as you did in PATCH 4/5. That would make the object a more direct representation of the XML data, and eliminate all of this complicated code dealing with "iplist" and nested loops.

} + virBufferAddLit(buf, "</dns>\n"); - } + }

virBufferAddLit(buf, "</ip>\n");

diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 5f47595..305ef0f 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -64,9 +64,18 @@ struct _virNetworkDNSTxtRecordsDef { char *value; };

+struct virNetworkDNSHostsDef { + virSocketAddr ip; + char *name;

Again - make name char** as you did in PATCH 4/5.

+} virNetworkDNSHostsDef; + +typedef struct virNetworkDNSHostsDef *virNetworkDNSHostsDefPtr; + struct virNetworkDNSDef { unsigned int ntxtrecords; + unsigned int nhosts; virNetworkDNSTxtRecordsDefPtr txtrecords; + virNetworkDNSHostsDefPtr hosts; } virNetworkDNSDef;

typedef struct virNetworkDNSDef *virNetworkDNSDefPtr; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4ad3143..99a61b0 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -434,13 +434,20 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,

This function is going to also need a virNetworkDefPtr arg if dns moves up a level.

goto cleanup; }

- if (! force&& virFileExists(dctx->hostsfile->path)) - return 0; + if (!(! force&& virFileExists(dctx->hostsfile->path))) {

That is much easier to understand if written as: if (force || !virFileExists(dctx->hostsfile->path)

+ for (i = 0; i< ipdef->nhosts; i++) { + virNetworkDHCPHostDefPtr host =&(ipdef->hosts[i]); + if ((host->mac)&& VIR_SOCKET_HAS_ADDR(&host->ip)) + dnsmasqAddDhcpHost(dctx, host->mac,&host->ip, host->name); + } + }

- for (i = 0; i< ipdef->nhosts; i++) { - virNetworkDHCPHostDefPtr host =&(ipdef->hosts[i]); - if ((host->mac)&& VIR_SOCKET_HAS_ADDR(&host->ip)) - dnsmasqAddDhcpHost(dctx, host->mac,&host->ip, host->name); + if (ipdef->dns) { + for (i = 0; i< ipdef->dns->nhosts; i++) { + virNetworkDNSHostsDefPtr host =&(ipdef->dns->hosts[i]); + if (VIR_SOCKET_HAS_ADDR(&host->ip)) + dnsmasqAddHost(dctx,&host->ip, host->name);

If you change the virNetworkDNSHostDefPtr to contain char **name instead of char *name, and dnsmasqAddHost to accept char** rather than char*, this can remain pretty much unchanged.

+ } }

if (dnsmasqSave(dctx)< 0) @@ -589,6 +596,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, if (dctx->hostsfile->nhosts) virCommandAddArgPair(cmd, "--dhcp-hostsfile", dctx->hostsfile->path); + VIR_DEBUG("ADDN HOSTS: %d => %p", dctx->addnhostsfile->nhosts, ipdef->dns);

Did you mean to leave this DEBUG in?

if (dctx->addnhostsfile->nhosts) virCommandAddArgPair(cmd, "--addn-hosts", dctx->addnhostsfile->path); diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv new file mode 100644 index 0000000..99dc724 --- /dev/null +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/default.pid --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/default.leases --dhcp-lease-max=253 --dhcp-no-override --dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile --addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.xml b/tests/networkxml2argvdata/nat-network-dns-hosts.xml new file mode 100644 index 0000000..35ee151 --- /dev/null +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.xml @@ -0,0 +1,19 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> +<host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> +</dhcp> +<dns> +<host ip='192.168.122.1'> +<hostname>host</hostname> +<hostname>gateway</hostname> +</host> +</dns> +</ip> +</network> diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index e3a8bb4..ce09206 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -112,6 +112,7 @@ mymain(int argc, char **argv) DO_TEST("netboot-network"); DO_TEST("netboot-proxy-network"); DO_TEST("nat-network-dns-txt-record"); + DO_TEST("nat-network-dns-hosts");

return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } diff --git a/tests/networkxml2xmlin/nat-network-dns-hosts.xml b/tests/networkxml2xmlin/nat-network-dns-hosts.xml new file mode 100644 index 0000000..fe545cf --- /dev/null +++ b/tests/networkxml2xmlin/nat-network-dns-hosts.xml @@ -0,0 +1,27 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> +<host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> +</dhcp> +<dns> +<host ip='192.168.122.1'> +<hostname>host</hostname> +<hostname>gateway</hostname> +</host> +</dns> +</ip> +<ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> +</ip> +<ip family='ipv4' address='10.24.10.1'> +</ip> +</network> diff --git a/tests/networkxml2xmlout/nat-network-dns-hosts.xml b/tests/networkxml2xmlout/nat-network-dns-hosts.xml new file mode 100644 index 0000000..fe545cf --- /dev/null +++ b/tests/networkxml2xmlout/nat-network-dns-hosts.xml @@ -0,0 +1,27 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> +<forward dev='eth1' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<ip address='192.168.122.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.122.2' end='192.168.122.254' /> +<host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> +<host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> +</dhcp> +<dns> +<host ip='192.168.122.1'> +<hostname>host</hostname> +<hostname>gateway</hostname> +</host> +</dns> +</ip> +<ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> +</ip> +<ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> +</ip> +<ip family='ipv4' address='10.24.10.1'> +</ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index beb00ef..f5c5715 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -91,6 +91,7 @@ mymain(int argc, char **argv) DO_TEST("netboot-network"); DO_TEST("netboot-proxy-network"); DO_TEST("nat-network-dns-txt-record"); + DO_TEST("nat-network-dns-hosts");

return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); }