network: Fix dnsmasq hostsfile creation logic and related tests

networkSaveDnsmasqHostsfile was added in 8fa9c2214247 (Apr 2010).
It has a force flag. If the dnsmasq hostsfile already exists force
needs to be true to overwrite it. networkBuildDnsmasqArgv sets force
to false, networkDefine sets it to true. This results in the
hostsfile being written only in networkDefine in the common case.
If no error occurred networkSaveDnsmasqHostsfile returns true and
networkBuildDnsmasqArgv adds the --dhcp-hostsfile to the dnsmasq
command line.

networkSaveDnsmasqHostsfile was changed in 89ae9849f744 (24 Jun 2011)
to return a new dnsmasqContext instead of reusing one. This change broke
the logic of the force flag as now networkSaveDnsmasqHostsfile returns
NULL on error, but the early return -- if force was not set and the
hostsfile exists -- returns 0. This turned the early return in an error
case and networkBuildDnsmasqArgv didn't add the --dhcp-hostsfile option
anymore if the hostsfile already exists. It did because networkDefine
created the hostsfile already.

Then 9d4e2845d498 fixed the return 0 case in networkSaveDnsmasqHostsfile
but didn't apply the force option correctly to the new addnhosts file.
Now force doesn't control an early return anymore, but influences the
handling of the hostsfile context creation and dnsmasqSave is always
called now. This commit also added test cases that reveal several
problems. First, the tests now calls functions that try to write the
dnsmasq config files to disk. If someone runs this tests as root this
might overwrite actively used dnsmasq config files, this is a no-go. Also
the tests depend on configure --localstatedir, this needs to be fixed as
well, because it makes the tests fail when localstatedir is different
from /var.

This patch does several things to fix this:

1) Move dnsmasqContext creation and saving out of networkBuildDnsmasqArgv
to the caller to separate the command line generation from the config
file writing. This makes the command line generation testable without the
risk of interfering with system files, because the tests just don't call
dnsmasqSave.

2) This refactoring of networkSaveDnsmasqHostsfile makes the force flag
useless as the saving happens somewhere else now. This fixes the wrong
usage of the force flag in combination with then newly added addnhosts
file by removing the force flag.

3) Adapt the wrong test cases to the correct behavior, by adding the
missing --dhcp-hostsfile option. Both affected tests contain DHCP host
elements but missed the necessary --dhcp-hostsfile option.

4) Rename networkSaveDnsmasqHostsfile to networkBuildDnsmasqHostsfile,
because it doesn't save the dnsmasqContext anymore.

5) Move all directory creations in dnsmasq context handling code from
the *New functions to dnsmasqSave to avoid directory creations in system
paths in the test cases.

6) Now that networkBuildDnsmasqArgv doesn't create the dnsmasqContext
anymore the test case can create one with the localstatedir that is
expected by the tests instead of the configure --localstatedir given one.

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 5d5282c48229185ca0bd40cd9e27ff36f1983ccf..f0cf122d3ce40c2765d7fd75548de4f6a5493ab9 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -436,27 +436,17 @@ networkShutdown(void) {
 }
 
 
-static dnsmasqContext*
-networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
-                            virNetworkDNSDefPtr dnsdef,
-                            char *name,
-                            bool force)
+static int
+networkBuildDnsmasqHostsfile(dnsmasqContext *dctx,
+                             virNetworkIpDefPtr ipdef,
+                             virNetworkDNSDefPtr dnsdef)
 {
     unsigned int i, j;
 
-    dnsmasqContext *dctx = dnsmasqContextNew(name,
-                                             DNSMASQ_STATE_DIR);
-    if (dctx == NULL) {
-        virReportOOMError();
-        goto cleanup;
-    }
-
-    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) {
@@ -469,15 +459,7 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
         }
     }
 
-    if (dnsmasqSave(dctx) < 0)
-        goto cleanup;
-
-    return dctx;
-
-cleanup:
-    dnsmasqContextFree(dctx);
-
-    return NULL;
+    return 0;
 }
 
 
@@ -485,12 +467,13 @@ static int
 networkBuildDnsmasqArgv(virNetworkObjPtr network,
                         virNetworkIpDefPtr ipdef,
                         const char *pidfile,
-                        virCommandPtr cmd) {
+                        virCommandPtr cmd,
+                        dnsmasqContext *dctx)
+{
     int r, ret = -1;
     int nbleases = 0;
     int ii;
     virNetworkIpDefPtr tmpipdef;
-    dnsmasqContext *dctx = NULL;
 
     /*
      * NB, be careful about syntax for dnsmasq options in long format.
@@ -621,14 +604,13 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
         if (network->def->domain)
            virCommandAddArg(cmd, "--expand-hosts");
 
-        if ((dctx = networkSaveDnsmasqHostsfile(ipdef, network->def->dns, network->def->name, false))) {
+        if (networkBuildDnsmasqHostsfile(dctx, ipdef, network->def->dns) >= 0) {
             if (dctx->hostsfile->nhosts)
                 virCommandAddArgPair(cmd, "--dhcp-hostsfile",
                                      dctx->hostsfile->path);
             if (dctx->addnhostsfile->nhosts)
                 virCommandAddArgPair(cmd, "--addn-hosts",
                                      dctx->addnhostsfile->path);
-            dnsmasqContextFree(dctx);
         }
 
         if (ipdef->tftproot) {
@@ -659,7 +641,7 @@ cleanup:
 
 int
 networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout,
-                                  char *pidfile)
+                                  char *pidfile, dnsmasqContext *dctx)
 {
     virCommandPtr cmd = NULL;
     int ret = -1, ii;
@@ -688,7 +670,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou
         return 0;
 
     cmd = virCommandNew(DNSMASQ);
-    if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd) < 0) {
+    if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx) < 0) {
         goto cleanup;
     }
 
@@ -708,6 +690,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
     char *pidfile = NULL;
     int ret = -1;
     int err;
+    dnsmasqContext *dctx = NULL;
 
     if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) {
         virReportSystemError(err,
@@ -734,8 +717,16 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
         goto cleanup;
     }
 
-    ret = networkBuildDhcpDaemonCommandLine(network,&cmd, pidfile);
-    if (ret<  0)
+    dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR);
+    if (dctx == NULL)
+        goto cleanup;
+
+    ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx);
+    if (ret < 0)
+        goto cleanup;
+
+    ret = dnsmasqSave(dctx);
+    if (ret < 0)
         goto cleanup;
 
     if (virCommandRun(cmd, NULL) < 0)
@@ -757,6 +748,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
 cleanup:
     VIR_FREE(pidfile);
     virCommandFree(cmd);
+    dnsmasqContextFree(dctx);
     return ret;
 }
 
@@ -2209,6 +2201,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
     virNetworkObjPtr network = NULL;
     virNetworkPtr ret = NULL;
     int ii;
+    dnsmasqContext* dctx = NULL;
 
     networkDriverLock(driver);
 
@@ -2255,10 +2248,11 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
         }
     }
     if (ipv4def) {
-        dnsmasqContext* dctx = networkSaveDnsmasqHostsfile(ipv4def, network->def->dns, network->def->name, true);
-        if (dctx == NULL)
+        dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR);
+        if (dctx == NULL ||
+            networkBuildDnsmasqHostsfile(dctx, ipv4def, network->def->dns) < 0 ||
+            dnsmasqSave(dctx) < 0)
             goto cleanup;
-        dnsmasqContextFree(dctx);
     }
 
     VIR_INFO("Defining network '%s'", network->def->name);
@@ -2266,6 +2260,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
 
 cleanup:
     virNetworkDefFree(def);
+    dnsmasqContextFree(dctx);
     if (network)
         virNetworkObjUnlock(network);
     networkDriverUnlock(driver);

diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index a106e3df571011a6cc165afbcc6a3e5ebc8c3694..2896c845845ab9602851862b74af1e9abdab8b76 100644 (file)
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -30,9 +30,12 @@
 # include "internal.h"
 # include "network_conf.h"
 # include "command.h"
+# include "dnsmasq.h"
 
 int networkRegister(void);
-int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile);
+int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
+                                      virCommandPtr *cmdout, char *pidfile,
+                                      dnsmasqContext *dctx);
 
 typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);
 

diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c
index 04a912aab3bc019e93b30dc1aacc52d579c31ace..4bdbb4416b5ef3e0a76f5aa096e5ac9e117636ed 100644 (file)
--- a/src/util/dnsmasq.c
+++ b/src/util/dnsmasq.c
@@ -142,7 +142,6 @@ static dnsmasqAddnHostsfile *
 addnhostsNew(const char *name,
              const char *config_dir)
 {
-    int err;
     dnsmasqAddnHostsfile *addnhostsfile;
 
     if (VIR_ALLOC(addnhostsfile) < 0) {
@@ -159,12 +158,6 @@ addnhostsNew(const char *name,
         goto error;
     }
 
-    if ((err = virFileMakePath(config_dir))) {
-        virReportSystemError(err, _("cannot create config directory '%s'"),
-                             config_dir);
-        goto error;
-    }
-
     return addnhostsfile;
 
  error:
@@ -344,7 +337,6 @@ static dnsmasqHostsfile *
 hostsfileNew(const char *name,
              const char *config_dir)
 {
-    int err;
     dnsmasqHostsfile *hostsfile;
 
     if (VIR_ALLOC(hostsfile) < 0) {
@@ -361,12 +353,6 @@ hostsfileNew(const char *name,
         goto error;
     }
 
-    if ((err = virFileMakePath(config_dir))) {
-        virReportSystemError(err, _("cannot create config directory '%s'"),
-                             config_dir);
-        goto error;
-    }
-
     return hostsfile;
 
  error:
@@ -468,6 +454,11 @@ dnsmasqContextNew(const char *network_name,
         return NULL;
     }
 
+    if (!(ctx->config_dir = strdup(config_dir))) {
+        virReportOOMError();
+        goto error;
+    }
+
     if (!(ctx->hostsfile = hostsfileNew(network_name, config_dir)))
         goto error;
     if (!(ctx->addnhostsfile = addnhostsNew(network_name, config_dir)))
@@ -492,6 +483,8 @@ dnsmasqContextFree(dnsmasqContext *ctx)
     if (!ctx)
         return;
 
+    VIR_FREE(ctx->config_dir);
+
     if (ctx->hostsfile)
         hostsfileFree(ctx->hostsfile);
     if (ctx->addnhostsfile)
@@ -546,8 +539,15 @@ dnsmasqAddHost(dnsmasqContext *ctx,
 int
 dnsmasqSave(const dnsmasqContext *ctx)
 {
+    int err;
     int ret = 0;
 
+    if ((err = virFileMakePath(ctx->config_dir))) {
+        virReportSystemError(err, _("cannot create config directory '%s'"),
+                             ctx->config_dir);
+        return -1;
+    }
+
     if (ctx->hostsfile)
         ret = hostsfileSave(ctx->hostsfile);
     if (ret == 0) {

diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h
index 3f6320a2fb38f5feb21a5456eb02bc24d336eb99..62f6f38f239af3dd2a55aea86c080adbc1f4c757 100644 (file)
--- a/src/util/dnsmasq.h
+++ b/src/util/dnsmasq.h
@@ -60,6 +60,7 @@ typedef struct
 
 typedef struct
 {
+    char                 *config_dir;
     dnsmasqHostsfile     *hostsfile;
     dnsmasqAddnHostsfile *addnhostsfile;
 } dnsmasqContext;

diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
index 9d745437dda3c77e911708b24da37680735d0244..be6ba4bff02f6921f51a85ca0638c105669411ab 100644 (file)
--- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
+++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
@@ -5,4 +5,5 @@
 --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-lease-max=253 --dhcp-no-override \
+--dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile\

diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv
index 181623f59c36a02a23cd1ff0a95353833395d018..d7faee201a1d037818ce8bf59c09808fff487148 100644 (file)
--- a/tests/networkxml2argvdata/nat-network.argv
+++ b/tests/networkxml2argvdata/nat-network.argv
@@ -4,4 +4,5 @@
 --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-lease-max=253 --dhcp-no-override \
+--dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile\

diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c
index 62de191eeb3e6b468b69d65847ef85bef0bcf574..4a11d6fea16bca00977156cad3ebbff7aac25b7a 100644 (file)
--- a/tests/networkxml2argvtest.c
+++ b/tests/networkxml2argvtest.c
@@ -24,6 +24,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
     virNetworkObjPtr obj = NULL;
     virCommandPtr cmd = NULL;
     char *pidfile = NULL;
+    dnsmasqContext *dctx = NULL;
 
     if (virtTestLoadFile(inxml, &inXmlData) < 0)
         goto fail;
@@ -38,8 +39,12 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
         goto fail;
 
     obj->def = dev;
+    dctx = dnsmasqContextNew(dev->name, "/var/lib/libvirt/dnsmasq");
 
-    if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile) < 0)
+    if (dctx == NULL)
+        goto fail;
+
+    if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0)
         goto fail;
 
     if (!(actual = virCommandToString(cmd)))
@@ -59,6 +64,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
     VIR_FREE(pidfile);
     virCommandFree(cmd);
     virNetworkObjFree(obj);
+    dnsmasqContextFree(dctx);
     return ret;
 }