I've attached a diff at the end that will address all the problems I've
brought up in my review. I can either squash that in and push the result
(once the rest of the series is reviewed and ready), or Michal can
squash it into his local tree and resubmit.
On 06/13/2011 12:55 PM, 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>
Signed-off-by: Michal Novotny<minovotn@xxxxxxxxxx>
---
docs/formatnetwork.html.in | 19 ++++
docs/schemas/network.rng | 13 +++
src/conf/network_conf.c | 97 ++++++++++++++++++++
src/conf/network_conf.h | 16 +++
src/network/bridge_driver.c | 21 ++++-
.../nat-network-dns-txt-record.xml | 24 +++++
.../nat-network-dns-txt-record.xml | 24 +++++
tests/networkxml2xmltest.c | 1 +
8 files changed, 214 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..008897d 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,22 @@
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.1</span>
s/0.9.1/0.9.3/
+ 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 which are
+ comma-separated which is allowed for the TXT records and it is represented a
+ single value.<span class="since">Since 0.9.1</span>
"a single string which can contain multiple values separated by commas."
seems more concise.
and again, s/0.9.1/0.9.3/
+</dd>
+
</dd><dt><code>dhcp</code></dt><dd>Also within the<code>ip</code> element there is an
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..3780af5 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"><text/></attribute>
I see that Daniel recommends restricting this to A-Z, a-z, and -. I
couldn't figure out from RFC 1035 if that's really what's intended (and
am not sure if there's another later RFC that amends 1035). But since
this is currently only used for tests, I guess it's okay to be too
restrictive; we can always loosen it up later if we need to.
+<attribute name="value"><text/></attribute>
+</element>
+</zeroOrMore>
+</element>
+</optional>
+
<!--<ip> element -->
<zeroOrMore>
<!-- The IP element sets up NAT'ing and an optional DHCP server
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index e4765ea..133ac84 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
There's no code here to critique, because you totally left it out - you
aren't free'ing the virNetworkDNSDef when you free the virNetworkDef.
@@ -435,6 +435,69 @@ 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)) {
+ 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"))) {
+ cur = cur->next;
+ continue;
+ }
+ if (!(value = virXMLPropString(cur, "value"))) {
+ VIR_FREE(name);
+ cur = cur->next;
+ continue;
+ }
These should be errors rather than silently ignoring the record.
+
+ if (strchr(name, ' ') != NULL) {
+ virNetworkReportError(VIR_ERR_XML_DETAIL,
+ _("TXT record names in DNS don't support spaces in 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;
+ def->ntxtrecords++;
+ }
+
+ cur = cur->next;
+ }
+
+ ret = 0;
+error:
+ if (ret< 0) {
+ name = NULL;
+ value = NULL;
Uh, I don't think you want those assignments there :-)
+ VIR_FREE(name);
+ VIR_FREE(value);
Also you need to free def here, since you're not returning it.
+ }
+ else
+ if (dnsdef != NULL)
+ *dnsdef = def;
This isn't proper style for libvirt. The "}", "else", and "if" should
be on the same line, and the else clause needs to have brackets around
it (since the if clause has brackets).
Also, dnsdef had *better* be non-NULL, or 1) you'll be leaking the
virNetworkDNSDef, and 2) it would be pointless to call this function,
since the whole purpose is to return the new DNSDef. Since you know it
will always be non-NULL, we can just leave the if condition out of the else.
+
+ return ret;
+}
+
+static int
virNetworkIPParseXML(const char *networkName,
virNetworkIpDefPtr def,
xmlNodePtr node,
@@ -584,6 +647,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
virNetworkDefPtr def;
char *tmp;
xmlNodePtr *ipNodes = NULL;
+ xmlNodePtr dnsNode = NULL;
int nIps;
if (VIR_ALLOC(def)< 0) {
@@ -641,6 +705,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;
@@ -695,6 +765,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
error:
virNetworkDefFree(def);
VIR_FREE(ipNodes);
+ VIR_FREE(dnsNode);
virXPathNode doesn't allocate anything, it just returns an existing
pointer, so dnsNode shouldn't be freed.
return NULL;
}
@@ -751,6 +822,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 +975,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..169bc08 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -456,7 +456,6 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
return 0;
}
-
static int
networkBuildDnsmasqArgv(virNetworkObjPtr network,
virNetworkIpDefPtr ipdef,
@@ -511,6 +510,26 @@ 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++) {
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ virBufferAsprintf(&buf, "%s,%s",
+ dns->txtrecords[i].name,
+ dns->txtrecords[i].value);
I can't see a reason for using virBufferAsprintf() here - since you have
a small, fixed number of args, virAsprintf() would do just as well.
+
+ if (virBufferError(&buf)) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ virCommandAddArgPair(cmd, "--txt-record", virBufferContentAndReset(&buf));
+ virBufferFreeAndReset(&buf);
This is redundant.
+ }
+ }
+
/*
* --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);
}
>From 3ba1ff01079e2bf8f6ec722afe335edae9c80b8a Mon Sep 17 00:00:00 2001
From: Laine Stump <laine@xxxxxxxxx>
Date: Mon, 13 Jun 2011 20:19:14 -0400
Subject: [PATCH] squash to TXT records 1/5
---
docs/formatnetwork.html.in | 7 +++----
docs/schemas/network.rng | 9 ++++++++-
src/conf/network_conf.c | 39 ++++++++++++++++++++++++++-------------
src/network/bridge_driver.c | 14 ++++++--------
4 files changed, 43 insertions(+), 26 deletions(-)
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 008897d..1cf7636 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -172,7 +172,7 @@
</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.1</span>
+ virtual network's DNS server. <span class="since">Since 0.9.3</span>
Currently supported elements are:
</dd>
<dt><code>txt</code></dt>
@@ -180,9 +180,8 @@
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 which is allowed for the TXT records and it is represented a
- single value. <span class="since">Since 0.9.1</span>
+ 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
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 3780af5..f6b3a4d 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -93,7 +93,7 @@
<element name="dns">
<zeroOrMore>
<element name="txt">
- <attribute name="name"><text/></attribute>
+ <attribute name="name"><ref name="dns-name"/></attribute>
<attribute name="value"><text/></attribute>
</element>
</zeroOrMore>
@@ -196,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 133ac84..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);
}
@@ -454,18 +470,19 @@ virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef,
if (cur->type == XML_ELEMENT_NODE &&
xmlStrEqual(cur->name, BAD_CAST "txt")) {
if (!(name = virXMLPropString(cur, "name"))) {
- cur = cur->next;
- continue;
+ virNetworkReportError(VIR_ERR_XML_DETAIL,
+ "%s", _("Missing required name attribute in dns txt record"));
+ goto error;
}
if (!(value = virXMLPropString(cur, "value"))) {
- VIR_FREE(name);
- cur = cur->next;
- continue;
+ 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,
- _("TXT record names in DNS don't support spaces in names (name is '%s')"), name);
+ _("spaces are not allowed in DNS TXT record names (name is '%s')"), name);
goto error;
}
@@ -485,15 +502,12 @@ virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef,
ret = 0;
error:
if (ret < 0) {
- name = NULL;
- value = NULL;
VIR_FREE(name);
VIR_FREE(value);
- }
- else
- if (dnsdef != NULL)
+ virNetworkDNSDefFree(def);
+ } else {
*dnsdef = def;
-
+ }
return ret;
}
@@ -765,7 +779,6 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
error:
virNetworkDefFree(def);
VIR_FREE(ipNodes);
- VIR_FREE(dnsNode);
return NULL;
}
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 169bc08..a2cba05 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -515,18 +515,16 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
int i;
for (i = 0; i < dns->ntxtrecords; i++) {
- virBuffer buf = VIR_BUFFER_INITIALIZER;
- virBufferAsprintf(&buf, "%s,%s",
- dns->txtrecords[i].name,
- dns->txtrecords[i].value);
-
- if (virBufferError(&buf)) {
+ char *record = NULL;
+ if (virAsprintf(&record, "%s,%s",
+ dns->txtrecords[i].name,
+ dns->txtrecords[i].value) < 0) {
virReportOOMError();
goto cleanup;
}
- virCommandAddArgPair(cmd, "--txt-record", virBufferContentAndReset(&buf));
- virBufferFreeAndReset(&buf);
+ virCommandAddArgPair(cmd, "--txt-record", record);
+ VIR_FREE(record);
}
}
--
1.7.3.4
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list