On 06/14/2011 09:02 AM, Michal Novotny wrote:

This commit introduces the<dns> element and<txt> record for the virtual DNS network. The DNS TXT record can be defined using following syntax in the network XML file:

<dns> <txt name="example" value="example value" /> </dns>

Also, the Relax-NG scheme has been altered to allow the texts without spaces only for the name element and some nitpicks about memory free'ing have been fixed by Laine so therefore I'm adding Laine to the SOB clause ;-)

You won't be the first to have called me an SOB :-P

Signed-off-by: Michal Novotny<minovotn@redhat.com> Signed-off-by: Laine Stump<lstump@redhat.com> --- docs/formatnetwork.html.in | 18 +++ docs/schemas/network.rng | 20 ++++ src/conf/network_conf.c | 110 ++++++++++++++++++++ src/conf/network_conf.h | 16 +++ src/network/bridge_driver.c | 19 +++- .../nat-network-dns-txt-record.xml | 24 +++++ .../nat-network-dns-txt-record.xml | 24 +++++ tests/networkxml2xmltest.c | 1 + 8 files changed, 231 insertions(+), 1 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 589aaff..1cf7636 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -114,6 +114,9 @@ <pre> ... <mac address='00:16:3E:5D:C7:9E'/> +<dns> +<txt name="example" value="example value" /> +</dns> <ip address="192.168.122.1" netmask="255.255.255.0"> <dhcp> <range start="192.168.122.100" end="192.168.122.254" /> @@ -166,6 +169,21 @@ supported for IPv6 addresses, can only be specified on a single IPv4 address per network. <span class="since">Since 0.7.1</span> + +</dd><dt><code>dns</code></dt><dd> + The dns element of a network contains configuration information for the + virtual network's DNS server.<span class="since">Since 0.9.3</span> + Currently supported elements are: +</dd> +<dt><code>txt</code></dt> +<dd>A<code>dns</code> element can have 0 or more<code>txt</code> 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 + separated by commas.<span class="since">Since 0.9.3</span> +</dd> + </dd><dt><code>dhcp</code></dt><dd>Also within the<code>ip</code> element there is an

You got an extra </dd> in here, which you noticed and fixed in Patch 2/5. Better to just get it right here to begin with...

optional<code>dhcp</code> element. The presence of this element enables DHCP services on the virtual network. It will further diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 6d01b06..c42382e 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -87,6 +87,19 @@ </element> </optional>

+<!-- Define the DNS related elements like TXT records + and other features in the<dns> element --> +<optional> +<element name="dns"> +<zeroOrMore> +<element name="txt"> +<attribute name="name"><ref name="dns-name"/></attribute> +<attribute name="value"><text/></attribute> +</element> +</zeroOrMore> +</element> +</optional> + <!--<ip> element --> <zeroOrMore> <!-- The IP element sets up NAT'ing and an optional DHCP server @@ -183,4 +196,11 @@ </data> </define>

+<!-- a valid DNS name --> +<define name='dns-name'> +<data type='string'> +<param name="pattern">([a-zA-Z\-]+)</param> +</data> +</define> + </grammar> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index e4765ea..93e931f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -104,6 +104,20 @@ static void virNetworkIpDefClear(virNetworkIpDefPtr def) VIR_FREE(def->bootfile); }

+static void virNetworkDNSDefFree(virNetworkDNSDefPtr def) +{ + if (def) { + if (def->txtrecords) { + while (def->ntxtrecords--) { + VIR_FREE(def->txtrecords[def->ntxtrecords].name); + VIR_FREE(def->txtrecords[def->ntxtrecords].value); + } + VIR_FREE(def->txtrecords); + } + VIR_FREE(def); + } +} + void virNetworkDefFree(virNetworkDefPtr def) { int ii; @@ -121,6 +135,8 @@ void virNetworkDefFree(virNetworkDefPtr def) } VIR_FREE(def->ips);

+ virNetworkDNSDefFree(def->dns); + VIR_FREE(def); }

@@ -435,6 +451,67 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, }

static int +virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef, + xmlNodePtr node) +{ + xmlNodePtr cur; + int ret = -1; + char *name = NULL; + char *value = NULL; + virNetworkDNSDefPtr def = NULL; + + if (VIR_ALLOC(def)) {

Oops. I missed this the last time. The accepted practice for error checking is "< 0", not "!=0" (which is what you're implying here).

+ virReportOOMError(); + goto error; + } + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE&& + xmlStrEqual(cur->name, BAD_CAST "txt")) { + if (!(name = virXMLPropString(cur, "name"))) { + virNetworkReportError(VIR_ERR_XML_DETAIL, + "%s", _("Missing required name attribute in dns txt record")); + goto error; + } + if (!(value = virXMLPropString(cur, "value"))) { + virNetworkReportError(VIR_ERR_XML_DETAIL, + _("Missing required value attribute in dns txt record '%s'"), name); + goto error; + } + + if (strchr(name, ' ') != NULL) { + virNetworkReportError(VIR_ERR_XML_DETAIL, + _("spaces are not allowed in DNS TXT record names (name is '%s')"), name); + goto error; + } + + if (VIR_REALLOC_N(def->txtrecords, def->ntxtrecords + 1)< 0) { + virReportOOMError(); + goto error; + } + + def->txtrecords[def->ntxtrecords].name = name; + def->txtrecords[def->ntxtrecords].value = value;

Eww. I just realized that if you were to make one successful trip through the loop, then hit an error on "name" the next time through, value would still point to a string that was also pointed to by the txtrecords array, leading to a double free during the error recovery. To avoid this, you need to add: name = NULL; value = NULL; right here. (maybe the name=NULL; value=NULL; that was in the wrong place in the previous version of the patch used to be here, and accidentally got moved...)

+ def->ntxtrecords++; + } + + cur = cur->next; + } + + ret = 0; +error: + if (ret< 0) { + VIR_FREE(name); + VIR_FREE(value); + virNetworkDNSDefFree(def); + } else { + *dnsdef = def; + } + return ret; +} + +static int virNetworkIPParseXML(const char *networkName, virNetworkIpDefPtr def, xmlNodePtr node, @@ -584,6 +661,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) virNetworkDefPtr def; char *tmp; xmlNodePtr *ipNodes = NULL; + xmlNodePtr dnsNode = NULL; int nIps;

if (VIR_ALLOC(def)< 0) { @@ -641,6 +719,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->mac_specified = true; }

+ dnsNode = virXPathNode("./dns", ctxt); + if (dnsNode != NULL) { + if (virNetworkDNSDefParseXML(&def->dns, dnsNode)< 0) + goto error; + } + nIps = virXPathNodeSet("./ip", ctxt,&ipNodes); if (nIps< 0) goto error; @@ -751,6 +835,29 @@ cleanup: }

static int +virNetworkDNSDefFormat(virBufferPtr buf, + virNetworkDNSDefPtr def) +{ + int result = 0; + int i; + + if (def == NULL) + goto out; + + virBufferAddLit(buf, "<dns>\n"); + + for (i = 0 ; i< def->ntxtrecords ; i++) { + virBufferAsprintf(buf, "<txt name='%s' value='%s' />\n", + def->txtrecords[i].name, + def->txtrecords[i].value); + } + + virBufferAddLit(buf, "</dns>\n"); +out: + return result; +} + +static int virNetworkIpDefFormat(virBufferPtr buf, const virNetworkIpDefPtr def) { @@ -881,6 +988,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) if (def->domain) virBufferAsprintf(&buf, "<domain name='%s'/>\n", def->domain);

+ if (virNetworkDNSDefFormat(&buf, def->dns)< 0) + goto error; + for (ii = 0; ii< def->nips; ii++) { if (virNetworkIpDefFormat(&buf,&def->ips[ii])< 0) goto error; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 281124b..d0dac02 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 { @@ -101,6 +115,8 @@ struct _virNetworkDef {

size_t nips; virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */ + + virNetworkDNSDefPtr dns; /* ptr to dns related configuration */ };

typedef struct _virNetworkObj virNetworkObj; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5e4b4d9..a2cba05 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -456,7 +456,6 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef, return 0; }

-

superfluous change in whitespace.

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

+ if (network->def->dns != NULL) { + virNetworkDNSDefPtr dns = network->def->dns; + int i; + + for (i = 0; i< dns->ntxtrecords; i++) { + char *record = NULL; + if (virAsprintf(&record, "%s,%s", + dns->txtrecords[i].name, + dns->txtrecords[i].value)< 0) { + virReportOOMError(); + goto cleanup; + } + + virCommandAddArgPair(cmd, "--txt-record", record); + VIR_FREE(record); + } + } + /* * --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..bd16976 --- /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' /> +<dns> +<txt name='example' value='example value' /> +</dns> +<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/networkxml2xmlout/nat-network-dns-txt-record.xml b/tests/networkxml2xmlout/nat-network-dns-txt-record.xml new file mode 100644 index 0000000..bd16976 --- /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' /> +<dns> +<txt name='example' value='example value' /> +</dns> +<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/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 468785b..2cc8e56 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -86,6 +86,7 @@ mymain(void) 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); }

I'm attaching a patch that fixes the problems I indicated above

On 06/14/2011 09:02 AM, Michal Novotny wrote:

The regression testing done by comparison of command-line generated from the network XML file and the expected command-line arguments (read from file).

For the tests the pidfile should be set to NULL string, i.e. (null) since no real invocation of the command is being done, just the command-line is being generated and compared to the expected one.

It's really just a happy coincidence of glib's printf that the NULL pointer ends up producing the string (null). If this code were ever used somewhere without glib (I know, that's extremely unlikely), this could lead to a segfault instead. Better to fix the networkBuildDnsmasqArgv() to just not include --pidfile if that arg is NULL.

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- docs/formatnetwork.html.in | 4 +- src/Makefile.am | 11 ++- src/network/bridge_driver.c | 36 +++++-- src/network/bridge_driver.h | 3 + tests/Makefile.am | 10 ++ 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 | 108 ++++++++++++++++++++ 18 files changed, 260 insertions(+), 10 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/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 1cf7636..62e1e08 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -183,8 +183,8 @@ or commas. value is a single string that can contain multiple values separated by commas.<span class="since">Since 0.9.3</span> </dd> - -</dd><dt><code>dhcp</code></dt><dd>Also within the<code>ip</code> element there is an +<dt><code>dhcp</code></dt> +<dd>Also within the<code>ip</code> element there is an

As I commented in 1/5 - this chunk (which just fixes a problem introduced in 1/5) should be squashed into 1/5 rather than tacked on here.

optional<code>dhcp</code> element. The presence of this element enables DHCP services on the virtual network. It will further contain one or more<code>range</code> elements. The diff --git a/src/Makefile.am b/src/Makefile.am index 4f9bfc9..edaedec 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -530,6 +530,10 @@ libvirt_driver_la_LIBADD = $(NUMACTL_LIBS) $(GNUTLS_LIBS) $(DLOPEN_LIBS)

USED_SYM_FILES = libvirt_private.syms

+if WITH_NETWORK +USED_SYM_FILES += libvirt_network.syms +endif +

Looks like you hadn't yet seen my earlier review of 2/5 when you reposted - this is duplicate code (you do the same thing down below). BTW, although you had included it in earlier versions of your patchset, for some reason libvirt_network.syms was left out this time.

if WITH_TEST if WITH_DRIVER_MODULES mod_LTLIBRARIES += libvirt_driver_test.la @@ -1089,6 +1093,10 @@ if WITH_XENXS USED_SYM_FILES += libvirt_xenxs.syms endif

+if WITH_NETWORK +USED_SYM_FILES += libvirt_network.syms +endif + EXTRA_DIST += \ libvirt_public.syms \ libvirt_private.syms \ @@ -1099,7 +1107,8 @@ EXTRA_DIST += \ libvirt_daemon.syms \ libvirt_nwfilter.syms \ libvirt_vmx.syms \ - libvirt_xenxs.syms + libvirt_xenxs.syms \ + libvirt_network.syms

BUILT_SOURCES += libvirt.syms libvirt.def libvirt_qemu.def

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a2cba05..1fad160 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -631,12 +631,12 @@ cleanup: return ret; }

-static int -networkStartDhcpDaemon(virNetworkObjPtr network) +int +networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) { virCommandPtr cmd = NULL; - char *pidfile = NULL; - int ret = -1, err, ii; + int ret = -1, ii; virNetworkIpDefPtr ipdef;

network->dnsmasqPid = -1; @@ -661,6 +661,29 @@ networkStartDhcpDaemon(virNetworkObjPtr network) if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) return 0;

+ cmd = virCommandNew(DNSMASQ); + if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd)< 0) { + goto cleanup; + } + + if (cmdout) + *cmdout = cmd; + + ret = 0; +cleanup: + if (ret< 0) + virCommandFree(cmd); + return ret; +} + +static int +networkStartDhcpDaemon(virNetworkObjPtr network) +{ + virCommandPtr cmd = NULL; + char *pidfile = NULL; + int ret = -1; + int err; + if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) { virReportSystemError(err, _("cannot create directory %s"), @@ -686,10 +709,9 @@ networkStartDhcpDaemon(virNetworkObjPtr network) goto cleanup; }

- cmd = virCommandNew(DNSMASQ); - if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd)< 0) { + ret = networkBuildDhcpDaemonCommandLine(network,&cmd, pidfile); + if (ret< 0) goto cleanup; - }

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

As you discovered, and fixed in Patch 3/5, make check with your new tests will now fail if run as non-root, because the test will try to write a hosts file in /var. Because "make check" should pass at any random point in git history, I think this change to bridge_driver.c should be combined with the fix in 3/5 into a separate patch that comes before this one that's adding the new tests.

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 7ae50a2..40eb71e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -50,6 +50,7 @@ EXTRA_DIST = \ networkschematest \ networkxml2xmlin \ networkxml2xmlout \ + networkxml2argvdata \ nodedevschemadata \ nodedevschematest \ nodeinfodata \ @@ -109,6 +110,8 @@ endif

check_PROGRAMS += networkxml2xmltest

+check_PROGRAMS += networkxml2argvtest + check_PROGRAMS += nwfilterxml2xmltest

check_PROGRAMS += storagevolxml2xmltest storagepoolxml2xmltest @@ -220,6 +223,8 @@ endif

TESTS += networkxml2xmltest

+TESTS += networkxml2argvtest + TESTS += storagevolxml2xmltest storagepoolxml2xmltest

TESTS += nodedevxml2xmltest @@ -347,6 +352,11 @@ networkxml2xmltest_SOURCES = \ testutils.c testutils.h networkxml2xmltest_LDADD = $(LDADDS)

+networkxml2argvtest_SOURCES = \ + networkxml2argvtest.c \ + testutils.c testutils.h +networkxml2argvtest_LDADD = ../src/libvirt_driver_network.la $(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..dce6034 --- /dev/null +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --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 \ No newline at end of file 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..96fde34 --- /dev/null +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --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 --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 \ No newline at end of file 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..bd16976 --- /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' /> +<dns> +<txt name='example' value='example value' /> +</dns> +<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/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv new file mode 100644 index 0000000..ccffa67 --- /dev/null +++ b/tests/networkxml2argvdata/nat-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --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 \ No newline at end of file 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..565c41b --- /dev/null +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --domain example.com --pid-file=(null) --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 \ No newline at end of file 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..019367d --- /dev/null +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --domain example.com --pid-file=(null) --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 \ No newline at end of file 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..2b51d90 --- /dev/null +++ b/tests/networkxml2argvdata/routed-network.argv @@ -0,0 +1 @@ +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --conf-file= --except-interface lo --listen-address 192.168.122.1 \ No newline at end of file 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..a0e5fd3 --- /dev/null +++ b/tests/networkxml2argvtest.c @@ -0,0 +1,108 @@ +#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 int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { + char *inXmlData = NULL; + char *outArgvData = NULL; + char *actual = NULL; + int ret = -1; + virNetworkDefPtr dev = NULL; + virNetworkObjPtr obj = NULL; + virCommandPtr cmd = NULL; + char *pidfile = NULL; + + if (virtTestLoadFile(inxml,&inXmlData)< 0) + goto fail; + + if (virtTestLoadFile(outargv,&outArgvData)< 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)

Standard practice in libvirt code is to check "< 0" rather than "!= 0".

+ goto fail; + + if (!(actual = virCommandToString(cmd))) + goto fail; + + if (STRNEQ(outArgvData, actual)) { + virtTestDifference(stderr, outArgvData, actual); + goto fail; + } + + ret = 0; + + fail: + free(inXmlData); + free(outArgvData); + free(actual); + VIR_FREE(pidfile); + virCommandFree(cmd); + virNetworkObjFree(obj); + return ret; +} + +static int +testCompareXMLToArgvHelper(const void *data) +{ + int result = -1; + char *inxml = NULL; + char *outxml = NULL; + + if (virAsprintf(&inxml, "%s/networkxml2argvdata/%s.xml", + abs_srcdir, (const char*)data)< 0 || + virAsprintf(&outxml, "%s/networkxml2argvdata/%s.argv", + abs_srcdir, (const char*)data)< 0) { + goto cleanup; + } + + result = testCompareXMLToArgvFiles(inxml, outxml); + +cleanup: + free(inxml); + free(outxml); + + return result; +} + +static int +mymain(void) +{ + int ret = 0; + +#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)

I'm attaching a patch you can squash in that addresses all the issues I noted above *except* the part about merging the bridge_driver.c changes from 3/5 with the bridge_driver.c changes from this patch to create a new 2/5, then merging the test program changes from this patch with the test data changes from patch 3/5 to create a new 3/5. Once you've done that, the new 2/5 and 3/5 will be ACKable.

This patch moves the dnsmasqContext creation to the networkSaveDnsmasqHostsfile() and saves and passes the hostsfile to the dnsmasq only if applicable. AS I mentioned in the review of 2/5, the bridge_driver.c changes in this

On 06/14/2011 09:02 AM, Michal Novotny wrote: patch should be combined with the bridge_driver.c changes in PATCH 2/5, becoming the new 2/5, and the changes to the test data in this patch should be combined with the remainder of PATCH 2/5, becoming PATCH 3/5. This will ensure that make check succeeds at each step along the git history (which is very important if we want git bisect to be useful). Otherwise I don't see any issues in this patch.

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- src/network/bridge_driver.c | 38 +++++++++++--------- .../nat-network-dns-txt-record.argv | 2 +- tests/networkxml2argvdata/nat-network.argv | 2 +- 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1fad160..10bb928 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -434,13 +434,20 @@ networkShutdown(void) { }

-static int +static dnsmasqContext* networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef, - dnsmasqContext *dctx, + char *name, bool force) { unsigned int i;

+ dnsmasqContext *dctx = dnsmasqContextNew(name, + DNSMASQ_STATE_DIR); + if (dctx == NULL) { + virReportOOMError(); + goto cleanup; + } + if (! force&& virFileExists(dctx->hostsfile->path)) return 0;

@@ -451,9 +458,14 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef, }

if (dnsmasqSave(dctx)< 0) - return -1; + goto cleanup;

- return 0; + return dctx; + +cleanup: + dnsmasqContextFree(dctx); + + return NULL; }

static int @@ -465,6 +477,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, int nbleases = 0; int ii; virNetworkIpDefPtr tmpipdef; + dnsmasqContext *dctx = NULL;

/* * NB, be careful about syntax for dnsmasq options in long format. @@ -590,18 +603,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, if (ipdef->nranges || ipdef->nhosts) virCommandAddArg(cmd, "--dhcp-no-override");

- if (ipdef->nhosts> 0) { - dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, - DNSMASQ_STATE_DIR); - if (dctx == NULL) { - virReportOOMError(); - goto cleanup; - } - - if (networkSaveDnsmasqHostsfile(ipdef, dctx, false) == 0) { + if ((dctx = networkSaveDnsmasqHostsfile(ipdef, network->def->name, false))) { + if (dctx->hostsfile->nhosts) virCommandAddArgPair(cmd, "--dhcp-hostsfile", dctx->hostsfile->path); - } + dnsmasqContextFree(dctx); }

@@ -2230,11 +2236,9 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { } } if (ipv4def) { - dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); + dnsmasqContext* dctx = networkSaveDnsmasqHostsfile(ipv4def, network->def->name, true); if (dctx == NULL) goto cleanup; - - networkSaveDnsmasqHostsfile(ipv4def, dctx, true); dnsmasqContextFree(dctx); }

diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index 96fde34..90cd578 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -1 +1 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --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 --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 \ No newline at end of file +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --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 --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 \ No newline at end of file diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index ccffa67..54a34e9 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -1 +1 @@ -/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --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 \ No newline at end of file +/usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=(null) --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 \ No newline at end of file

On 06/14/2011 09:02 AM, Michal Novotny wrote:

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- docs/formatnetwork.html.in | 14 ++ src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 3 + src/util/dnsmasq.c | 285 ++++++++++++++++++++++++++++++++++++++++--- src/util/dnsmasq.h | 22 +++- 5 files changed, 305 insertions(+), 20 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 62e1e08..a036545 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -218,6 +218,20 @@ 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>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> +<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>

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 737cd31..71ef9cc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -194,6 +194,7 @@ virUnrefStream;

# dnsmasq.h dnsmasqAddDhcpHost; +dnsmasqAddHost; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 10bb928..f55f759 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -607,6 +607,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 2ba9355..04a912a 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,235 @@ dhcphostFree(dnsmasqDhcpHost *host) }

static void +addnhostFree(dnsmasqAddnHost *host) +{ + int i; + + for (i = 0; i< host->nhostnames; i++) + VIR_FREE(host->hostnames[i]); + VIR_FREE(host->hostnames); + 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, + const char *name) +{ + 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 +genericFileDelete(char *path) +{ + if (!virFileExists(path)) + return 0; + + if (unlink(path)< 0) { + virReportSystemError(errno, _("cannot remove config file '%s'"), + path); + return -1; + } + + return 0; +} + +static void hostsfileFree(dnsmasqHostsfile *hostsfile) { unsigned int i; @@ -220,21 +450,6 @@ hostsfileSave(dnsmasqHostsfile *hostsfile) return 0; }

-static int -hostsfileDelete(dnsmasqHostsfile *hostsfile) -{ - if (!virFileExists(hostsfile->path)) - return 0; - - if (unlink(hostsfile->path)< 0) { - virReportSystemError(errno, _("cannot remove config file '%s'"), - hostsfile->path); - return -1; - } - - return 0; -} - /** * dnsmasqContextNew: * @@ -255,6 +470,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 +494,8 @@ dnsmasqContextFree(dnsmasqContext *ctx)

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

VIR_FREE(ctx); } @@ -300,6 +519,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 +546,16 @@ dnsmasqAddDhcpHost(dnsmasqContext *ctx, int dnsmasqSave(const dnsmasqContext *ctx) { + int ret = 0; + if (ctx->hostsfile) - return hostsfileSave(ctx->hostsfile); + ret = hostsfileSave(ctx->hostsfile); + if (ret == 0) { + if (ctx->addnhostsfile) + ret = addnhostsSave(ctx->addnhostsfile); + }

- return 0; + return ret; }

@@ -325,10 +568,14 @@ dnsmasqSave(const dnsmasqContext *ctx) int dnsmasqDelete(const dnsmasqContext *ctx) { + int ret = 0; + if (ctx->hostsfile) - return hostsfileDelete(ctx->hostsfile); + ret = genericFileDelete(ctx->hostsfile->path); + if (ctx->addnhostsfile) + ret = genericFileDelete(ctx->addnhostsfile->path);

- return 0; + return ret; }

/** 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;

The way you've defined your struct here is how you need to define virNetworkDNSHostsDef in the next patch.

+ +} 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);

ACK.

On 06/14/2011 09:02 AM, Michal Novotny wrote:

This commit introduces names definition for the DNS hosts file using the following syntax:

<dns> <host ip="192.168.1.1"> <name>alias1</name> <name>alias2</name> </host> </dns>

Signed-off-by: Michal Novotny<minovotn@redhat.com> --- docs/formatnetwork.html.in | 9 +-- docs/schemas/network.rng | 8 ++ src/conf/network_conf.c | 122 +++++++++++++++++++++ src/conf/network_conf.h | 9 ++ src/network/bridge_driver.c | 24 +++-- tests/networkxml2xmlin/nat-network-dns-hosts.xml | 14 +++ tests/networkxml2xmlout/nat-network-dns-hosts.xml | 14 +++ tests/networkxml2xmltest.c | 1 + 8 files changed, 185 insertions(+), 16 deletions(-) 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 a036545..f17cc63 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -224,15 +224,8 @@ 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> +<span class="since">Since 0.9.3</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 c42382e..05f45e5 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -97,6 +97,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> +</oneOrMore> +</element> +</zeroOrMore> </element> </optional>

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 93e931f..edc9186 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -114,6 +114,13 @@ static void virNetworkDNSDefFree(virNetworkDNSDefPtr def) } VIR_FREE(def->txtrecords); } + if (def->nhosts) { + while (def->nhosts--) { + VIR_FREE(def->hosts[def->nhosts].name); + VIR_FREE(def->hosts[def->nhosts].ip);

hosts[x].ip isn't a virSocketAddrPtr, it's a virSocketAddr, so it doesn't need to be freed (this doesn't even compile).

+ } + VIR_FREE(def->hosts); + } VIR_FREE(def); } } @@ -451,6 +458,53 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, }

static int +virNetworkDNSHostsDefParseXML(virNetworkDNSDefPtr def, + xmlNodePtr node, + virSocketAddr ip) +{ + xmlNodePtr cur; + int ret = -1; + + if (def->hosts == NULL) { + if (VIR_ALLOC(def->hosts)< 0) { + virReportOOMError(); + goto out; + } + def->nhosts = 0; + } + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE&& + xmlStrEqual(cur->name, BAD_CAST "hostname")) { + if (cur->children != NULL) { + char *hostname; + + hostname = strdup((char *)cur->children->content); + + if (VIR_REALLOC_N(def->hosts, def->nhosts + 1)< 0) { + VIR_FREE(hostname); + ret = -1;

ret = -1 is unnecessary here - it was initialized to that value and hasn't been modified.

+ virReportOOMError(); + goto out; + } + + def->hosts[def->nhosts].name = hostname; + def->hosts[def->nhosts].ip = ip; + def->nhosts++; + } + } + + cur = cur->next; + } + + ret = 0; + +out:

libvirt prefers to use "error" for this label when it's always used for error returns.

+ return ret; +} + +static int virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef, xmlNodePtr node) { @@ -494,6 +548,27 @@ virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef, def->txtrecords[def->ntxtrecords].name = name; def->txtrecords[def->ntxtrecords].value = value; def->ntxtrecords++; + } else if (cur->type == XML_ELEMENT_NODE&& + xmlStrEqual(cur->name, BAD_CAST "host")) { + char *ip; + virSocketAddr inaddr; + memset(&inaddr, 0, sizeof(inaddr));

In some other examples in this file (eg, the bootp "server" attribute), the address is optional, so inaddr must be initialized. In this case, though, the IP address is not optional, so it doesn't need to be initialized (similar to dhcp static host ip's).

+ + if (!(ip = virXMLPropString(cur, "ip"))) { + cur = cur->next; + continue; + }

I think the above code shouldn't be there - the ip address is required, not optional, and the code just below will error out if ip is NULL.

+ if ((ip == NULL) || + (virSocketParseAddr(ip,&inaddr, AF_UNSPEC)< 0)) { + virNetworkReportError(VIR_ERR_XML_DETAIL, + _("Missing IP address in DNS host definition")); + VIR_FREE(ip); + goto error; + } + VIR_FREE(ip); + ret = virNetworkDNSHostsDefParseXML(def, cur, inaddr);

You know, if you're going to parse the rest of the host element in a function, you really should parse the ip attribute in that function too, rather than sticking it up here.

+ if (ret< 0) + goto error; }

cur = cur->next; @@ -852,6 +927,53 @@ virNetworkDNSDefFormat(virBufferPtr buf, def->txtrecords[i].value); }

+ if (def->nhosts) { + int ii, j; + char **iplist = NULL; + int iplist_size = 0; + bool in_list; + + if (VIR_ALLOC(iplist)< 0) { + virReportOOMError(); + result = -1; + goto out; + } + + for (ii = 0 ; ii< def->nhosts; ii++) { + char *ip = virSocketFormatAddr(&def->hosts[ii].ip); + in_list = false; + + for (j = 0; j< iplist_size; j++) + if (STREQ(iplist[j], ip)) + in_list = true;

This function is unnecessarily complicated by the duplication of one piece of data into multiple places (the ip address). I think it would be better if the data structure exactly mimicked the XML - an array of structs that each has one IP address and an array of hostnames.

+ + if (!in_list) { + virBufferAsprintf(buf, "<host ip='%s'>\n", ip); + + for (j = 0 ; j< def->nhosts; j++) { + char *thisip = virSocketFormatAddr(&def->hosts[j].ip); + if (STREQ(ip, thisip)) + virBufferAsprintf(buf, "<hostname>%s</hostname>\n", + def->hosts[j].name); + } + virBufferAsprintf(buf, "</host>\n"); + + if (VIR_REALLOC_N(iplist, iplist_size + 1)< 0) { + virReportOOMError(); + result = -1; + goto out; + } + + iplist[iplist_size] = ip; + iplist_size++; + } + } + + for (j = 0; j< iplist_size; j++) + VIR_FREE(iplist[j]); + VIR_FREE(iplist); + } + virBufferAddLit(buf, "</dns>\n"); out: return result; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index d0dac02..50b3713 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; +} virNetworkDNSHostsDef; + +typedef struct virNetworkDNSHostsDef *virNetworkDNSHostsDefPtr; + struct virNetworkDNSDef { unsigned int ntxtrecords; + unsigned int nhosts; virNetworkDNSTxtRecordsDefPtr txtrecords; + virNetworkDNSHostsDefPtr hosts;

The "nXXX" and "XXX" members are usually kept next to each other, rather than grouping "nXXX", "nYYY", etc. together.

} virNetworkDNSDef;

typedef struct virNetworkDNSDef *virNetworkDNSDefPtr; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f55f759..d528875 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -436,6 +436,7 @@ networkShutdown(void) {

static dnsmasqContext* networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef, + virNetworkDNSDefPtr dnsdef, char *name, bool force) { @@ -448,13 +449,20 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef, goto cleanup; }

- if (! force&& virFileExists(dctx->hostsfile->path)) - return 0; + 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 (dnsdef) { + for (i = 0; i< dnsdef->nhosts; i++) { + virNetworkDNSHostsDefPtr host =&(dnsdef->hosts[i]); + if (VIR_SOCKET_HAS_ADDR(&host->ip)) + dnsmasqAddHost(dctx,&host->ip, host->name); + } }

if (dnsmasqSave(dctx)< 0) @@ -603,7 +611,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, if (ipdef->nranges || ipdef->nhosts) virCommandAddArg(cmd, "--dhcp-no-override");

- if ((dctx = networkSaveDnsmasqHostsfile(ipdef, network->def->name, false))) { + if ((dctx = networkSaveDnsmasqHostsfile(ipdef, network->def->dns, network->def->name, false))) { if (dctx->hostsfile->nhosts) virCommandAddArgPair(cmd, "--dhcp-hostsfile", dctx->hostsfile->path); @@ -2239,7 +2247,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { } } if (ipv4def) { - dnsmasqContext* dctx = networkSaveDnsmasqHostsfile(ipv4def, network->def->name, true); + dnsmasqContext* dctx = networkSaveDnsmasqHostsfile(ipv4def, network->def->dns, network->def->name, true); if (dctx == NULL) goto cleanup; dnsmasqContextFree(dctx); diff --git a/tests/networkxml2xmlin/nat-network-dns-hosts.xml b/tests/networkxml2xmlin/nat-network-dns-hosts.xml new file mode 100644 index 0000000..9a83fed --- /dev/null +++ b/tests/networkxml2xmlin/nat-network-dns-hosts.xml @@ -0,0 +1,14 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid> +<forward dev='eth0' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<dns> +<host ip='192.168.122.1'> +<hostname>host</hostname> +<hostname>gateway</hostname> +</host> +</dns> +<ip address='192.168.122.1' netmask='255.255.255.0'> +</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..9a83fed --- /dev/null +++ b/tests/networkxml2xmlout/nat-network-dns-hosts.xml @@ -0,0 +1,14 @@ +<network> +<name>default</name> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid> +<forward dev='eth0' mode='nat'/> +<bridge name='virbr0' stp='on' delay='0' /> +<dns> +<host ip='192.168.122.1'> +<hostname>host</hostname> +<hostname>gateway</hostname> +</host> +</dns> +<ip address='192.168.122.1' netmask='255.255.255.0'> +</ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 2cc8e56..065166d 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -87,6 +87,7 @@ mymain(void) 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); }

I'm attaching a patch to squash in that addresses all the issues I've noted above *except* changing the hosts def to have one ip address and multiple hostnames (and associated simplification of the code). Once you squash in my patch, and fix the data representation I can ACK this patch too.