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

Re: [libvirt] [PATCH v5 5/5] Network: Add support for DNS hosts definition to the network XML



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.
>From 99a4d33ae4bab241872d4a018020f32fa40df56a Mon Sep 17 00:00:00 2001
From: Laine Stump <laine laine org>
Date: Thu, 23 Jun 2011 00:53:01 -0400
Subject: [PATCH] squash into 5/5

---
 src/conf/network_conf.c |   45 ++++++++++++++++++---------------------------
 src/conf/network_conf.h |    2 +-
 2 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d4f6912..4bc486e 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -115,10 +115,8 @@ static void virNetworkDNSDefFree(virNetworkDNSDefPtr def)
             VIR_FREE(def->txtrecords);
         }
         if (def->nhosts) {
-            while (def->nhosts--) {
+            while (def->nhosts--)
                 VIR_FREE(def->hosts[def->nhosts].name);
-                VIR_FREE(def->hosts[def->nhosts].ip);
-            }
             VIR_FREE(def->hosts);
         }
         VIR_FREE(def);
@@ -459,20 +457,30 @@ virNetworkDHCPRangeDefParseXML(const char *networkName,
 
 static int
 virNetworkDNSHostsDefParseXML(virNetworkDNSDefPtr def,
-                              xmlNodePtr node,
-                              virSocketAddr ip)
+                              xmlNodePtr node)
 {
     xmlNodePtr cur;
+    char *ip;
+    virSocketAddr inaddr;
     int ret = -1;
 
     if (def->hosts == NULL) {
         if (VIR_ALLOC(def->hosts) < 0) {
             virReportOOMError();
-            goto out;
+            goto error;
         }
         def->nhosts = 0;
     }
 
+    if (!(ip = virXMLPropString(node, "ip")) ||
+        (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);
+
     cur = node->children;
     while (cur != NULL) {
         if (cur->type == XML_ELEMENT_NODE &&
@@ -484,13 +492,12 @@ virNetworkDNSHostsDefParseXML(virNetworkDNSDefPtr def,
 
                   if (VIR_REALLOC_N(def->hosts, def->nhosts + 1) < 0) {
                       VIR_FREE(hostname);
-                      ret = -1;
                       virReportOOMError();
-                      goto out;
+                      goto error;
                   }
 
                   def->hosts[def->nhosts].name = hostname;
-                  def->hosts[def->nhosts].ip = ip;
+                  def->hosts[def->nhosts].ip = inaddr;
                   def->nhosts++;
               }
         }
@@ -500,7 +507,7 @@ virNetworkDNSHostsDefParseXML(virNetworkDNSDefPtr def,
 
     ret = 0;
 
-out:
+error:
     return ret;
 }
 
@@ -552,23 +559,7 @@ virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef,
             value = NULL;
         } else if (cur->type == XML_ELEMENT_NODE &&
             xmlStrEqual(cur->name, BAD_CAST "host")) {
-            char *ip;
-            virSocketAddr inaddr;
-            memset(&inaddr, 0, sizeof(inaddr));
-
-            if (!(ip = virXMLPropString(cur, "ip"))) {
-                cur = cur->next;
-                continue;
-            }
-            if ((ip == NULL) ||
-                (virSocketParseAddr(ip, &inaddr, AF_UNSPEC) < 0)) {
-                virNetworkReportError(VIR_ERR_XML_DETAIL,
-                                      _("Missing IP address in DNS host definition"));
-                VIR_FREE(ip);
-                goto error;
-            }
-            VIR_FREE(ip);
-            ret = virNetworkDNSHostsDefParseXML(def, cur, inaddr);
+            ret = virNetworkDNSHostsDefParseXML(def, cur);
             if (ret < 0)
                 goto error;
         }
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 50b3713..2341df4 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -73,8 +73,8 @@ typedef struct virNetworkDNSHostsDef *virNetworkDNSHostsDefPtr;
 
 struct virNetworkDNSDef {
     unsigned int ntxtrecords;
-    unsigned int nhosts;
     virNetworkDNSTxtRecordsDefPtr txtrecords;
+    unsigned int nhosts;
     virNetworkDNSHostsDefPtr hosts;
 } virNetworkDNSDef;
 
-- 
1.7.3.4


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