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

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



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 @@
              &lt;host mac="00:16:3e:77:e2:ed" name="foo.example.com" ip="192.168.122.10" /&gt;
              &lt;host mac="00:16:3e:3e:a9:1a" name="bar.example.com" ip="192.168.122.11" /&gt;
            &lt;/dhcp&gt;
+&lt;dns&gt;
+&lt;txt-record name="example" value="example value" /&gt;
+&lt;/dns&gt;

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>


          &lt;/ip&gt;
        &lt;/network&gt;</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);
  }


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