[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 1/5] Add TXT record support for virtual DNS service



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>
          ...
          &lt;mac address='00:16:3E:5D:C7:9E'/&gt;
+&lt;dns&gt;
+&lt;txt name="example" value="example value" /&gt;
+&lt;/dns&gt;
          &lt;ip address="192.168.122.1" netmask="255.255.255.0"&gt;
            &lt;dhcp&gt;
              &lt;range start="192.168.122.100" end="192.168.122.254" /&gt;
@@ -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
>From 1bf72a21c57cd3ab439d7337f7b13840cedcee5e Mon Sep 17 00:00:00 2001
From: Laine Stump <laine laine org>
Date: Tue, 14 Jun 2011 20:08:53 -0400
Subject: [PATCH] squash into txt record 1/5

---
 docs/formatnetwork.html.in  |    4 ++--
 src/conf/network_conf.c     |    4 +++-
 src/network/bridge_driver.c |    1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

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
         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/conf/network_conf.c b/src/conf/network_conf.c
index 93e931f..d8f1e25 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -460,7 +460,7 @@ virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef,
     char *value = NULL;
     virNetworkDNSDefPtr def = NULL;
 
-    if (VIR_ALLOC(def)) {
+    if (VIR_ALLOC(def) < 0) {
         virReportOOMError();
         goto error;
     }
@@ -494,6 +494,8 @@ virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef,
             def->txtrecords[def->ntxtrecords].name = name;
             def->txtrecords[def->ntxtrecords].value = value;
             def->ntxtrecords++;
+            name = NULL;
+            value = NULL;
         }
 
         cur = cur->next;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 510a38f..dc143db 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -455,6 +455,7 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
     return 0;
 }
 
+
 static int
 networkBuildDnsmasqArgv(virNetworkObjPtr network,
                         virNetworkIpDefPtr ipdef,
-- 
1.7.3.4


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]