Re: [libvirt] PATCH: Allow virtual networks to survive daemon restarts

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

 



On Wed, Jan 14, 2009 at 08:55:46AM +0000, Mark McLoughlin wrote:
> On Tue, 2009-01-13 at 20:49 +0000, Daniel P. Berrange wrote:
> > Currently when we shutdown  the virtual networks are all shutdown too.
> > This is less than useful if we're letting guest VMs hang around post
> > shutdown of libvirtd, because it means we're tearing their network
> > connection out from under them. This patch fixes that allowing networks
> > to survive restarts, and be re-detected next time around.
> > 
> > When starting a virtual network we write the live config into
> > 
> >   /var/lib/libvirt/network/$NAME.xml
> > 
> > This is because the bridge device name is potentially auto-generated
> > and we need to keep track of that
> > 
> > We change dnsmasq args to include an explicit pidfile location
> > 
> >   /var/run/libvirt/network/$NAME.pid
> > 
> > and also tell it to put itself into the background - ie daemonize. This
> > is because we want dnsmasq to survive the daemon.
> > 
> > Now, when libvirtd starts up it
> > 
> >  - Looks for the live config, and if found loads it. 
> >  - Calls a new method brHasBridge() to see if its desired bridge
> >    actually exists (and thus whether the network is running).
> >    If it exists,the network is marked active
> >  - If DHCP is configured, then reads the dnsmasq PIDfile, and sends
> >    kill($PID, 0) to check if the process is actually alive
> > 
> > In addition I cleanup the network code to remove the configFile and
> > autostartLink fields in virNetworkObjPtr, so it matches virDomaiObjPtr
> > usage.
> > 
> > With all this applied you can now restart the daemon, and virbr0 is
> > left happily running.
> 
> It all sounds and looks good to me. Some comments below, but nothing
> major.

Here's an updated patch with all your comments incorporated.

 libvirt.spec.in          |   31 ++++-
 src/Makefile.am          |   24 +++-
 src/bridge.c             |   37 ++++++
 src/bridge.h             |    2 
 src/libvirt_bridge.syms  |    1 
 src/libvirt_private.syms |    2 
 src/network_conf.c       |  130 +++++++++++++----------
 src/network_conf.h       |   18 ++-
 src/network_driver.c     |  260 +++++++++++++++++++++++++++++++++++------------
 9 files changed, 370 insertions(+), 135 deletions(-)


Daniel

diff --git a/libvirt.spec.in b/libvirt.spec.in
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -11,6 +11,7 @@
 %define with_python    0%{!?_without_python:1}
 %define with_libvirtd  0%{!?_without_libvirtd:1}
 %define with_uml       0%{!?_without_uml:1}
+%define with_network   0%{!?_without_network:1}
 
 # Xen is available only on i386 x86_64 ia64
 %ifnarch i386 i686 x86_64 ia64
@@ -207,6 +208,10 @@ of recent versions of Linux (and other O
 %define _without_uml --without-uml
 %endif
 
+%if ! %{with_network}
+%define _without_network --without-network
+%endif
+
 %configure %{?_without_xen} \
            %{?_without_qemu} \
            %{?_without_openvz} \
@@ -217,6 +222,7 @@ of recent versions of Linux (and other O
            %{?_without_python} \
            %{?_without_libvirtd} \
            %{?_without_uml} \
+           %{?_without_network} \
            --with-init-script=redhat \
            --with-qemud-pid-file=%{_localstatedir}/run/libvirt_qemud.pid \
            --with-remote-file=%{_localstatedir}/run/libvirtd.pid
@@ -232,11 +238,6 @@ rm -f $RPM_BUILD_ROOT%{_libdir}/*.la
 rm -f $RPM_BUILD_ROOT%{_libdir}/*.a
 rm -f $RPM_BUILD_ROOT%{_libdir}/python*/site-packages/*.la
 rm -f $RPM_BUILD_ROOT%{_libdir}/python*/site-packages/*.a
-install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/run/libvirt/
-# Default dir for disk images defined in SELinux policy
-install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/lib/libvirt/images/
-# Default dir for kernel+initrd images defnied in SELinux policy
-install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/lib/libvirt/boot/
 
 %if %{with_qemu}
 # We don't want to install /etc/libvirt/qemu/networks in the main %files list
@@ -338,11 +339,31 @@ fi
 %endif
 
 %dir %{_localstatedir}/run/libvirt/
+
 %dir %{_localstatedir}/lib/libvirt/
 %dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/images/
 %dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/boot/
 
 %if %{with_qemu}
+%dir %{_localstatedir}/run/libvirt/qemu/
+%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/qemu/
+%endif
+%if %{with_lxc}
+%dir %{_localstatedir}/run/libvirt/lxc/
+%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/lxc/
+%endif
+%if %{with_uml}
+%dir %{_localstatedir}/run/libvirt/uml/
+%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/uml/
+%endif
+%if %{with_network}
+%dir %{_localstatedir}/run/libvirt/network/
+%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/network/
+%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/iptables/filter/
+%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/iptables/nat/
+%endif
+
+%if %{with_qemu}
 %{_datadir}/augeas/lenses/libvirtd_qemu.aug
 %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
 %endif
diff --git a/src/Makefile.am b/src/Makefile.am
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -589,9 +589,29 @@ endif
 endif
 EXTRA_DIST += $(LXC_CONTROLLER_SOURCES)
 
-# Create the /var/cache/libvirt directory when installing.
 install-exec-local:
-	$(MKDIR_P) $(DESTDIR)$(localstatedir)/cache/libvirt
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/images"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/boot"
+if WITH_QEMU
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/qemu"
+endif
+if WITH_LXC
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/lxc"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/lxc"
+endif
+if WITH_UML
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/uml"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/uml"
+endif
+if WITH_NETWORK
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/iptables/filter"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/iptables/nat"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/network"
+	$(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/network"
+endif
+
 
 CLEANFILES = *.gcov .libs/*.gcda .libs/*.gcno *.gcno *.gcda
 DISTCLEANFILES = $(BUILT_SOURCES)
diff --git a/src/bridge.c b/src/bridge.c
--- a/src/bridge.c
+++ b/src/bridge.c
@@ -163,6 +163,43 @@ int brAddBridge (brControl *ctl ATTRIBUT
 }
 #endif
 
+#ifdef SIOCBRDELBR
+int
+brHasBridge(brControl *ctl,
+            const char *name)
+{
+    struct ifreq ifr;
+    int len;
+
+    if (!ctl || !name) {
+        errno = EINVAL;
+        return -1;
+    }
+
+    if ((len = strlen(name)) >= BR_IFNAME_MAXLEN) {
+        errno = EINVAL;
+        return -1;
+    }
+
+    memset(&ifr, 0, sizeof(struct ifreq));
+
+    strncpy(ifr.ifr_name, name, len);
+    ifr.ifr_name[len] = '\0';
+
+    if (ioctl(ctl->fd, SIOCGIFFLAGS, &ifr))
+        return -1;
+
+    return 0;
+}
+#else
+int
+brHasBridge(brControl *ctl,
+            const char *name)
+{
+    return EINVAL;
+}
+#endif
+
 /**
  * brDeleteBridge:
  * @ctl: bridge control pointer
diff --git a/src/bridge.h b/src/bridge.h
--- a/src/bridge.h
+++ b/src/bridge.h
@@ -50,6 +50,8 @@ int     brAddBridge             (brContr
                                  char **name);
 int     brDeleteBridge          (brControl *ctl,
                                  const char *name);
+int     brHasBridge             (brControl *ctl,
+                                 const char *name);
 
 int     brAddInterface          (brControl *ctl,
                                  const char *bridge,
diff --git a/src/libvirt_bridge.syms b/src/libvirt_bridge.syms
--- a/src/libvirt_bridge.syms
+++ b/src/libvirt_bridge.syms
@@ -9,6 +9,7 @@ brAddBridge;
 brAddInterface;
 brAddTap;
 brDeleteBridge;
+brHasBridge;
 brInit;
 brSetEnableSTP;
 brSetForwardDelay;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -190,6 +190,7 @@ virFree;
 
 # network_conf.h
 virNetworkAssignDef;
+virNetworkConfigFile;
 virNetworkDefFormat;
 virNetworkDefFree;
 virNetworkDefParseFile;
@@ -202,6 +203,7 @@ virNetworkLoadAllConfigs;
 virNetworkObjListFree;
 virNetworkDefParseNode;
 virNetworkRemoveInactive;
+virNetworkSaveConfigXML;
 virNetworkSaveConfig;
 virNetworkObjLock;
 virNetworkObjUnlock;
diff --git a/src/network_conf.c b/src/network_conf.c
--- a/src/network_conf.c
+++ b/src/network_conf.c
@@ -125,9 +125,6 @@ void virNetworkObjFree(virNetworkObjPtr 
     virNetworkDefFree(net->def);
     virNetworkDefFree(net->newDef);
 
-    VIR_FREE(net->configFile);
-    VIR_FREE(net->autostartLink);
-
     virMutexDestroy(&net->lock);
 
     VIR_FREE(net);
@@ -641,31 +638,17 @@ char *virNetworkDefFormat(virConnectPtr 
     return NULL;
 }
 
-int virNetworkSaveConfig(virConnectPtr conn,
-                         const char *configDir,
-                         const char *autostartDir,
-                         virNetworkObjPtr net)
+int virNetworkSaveXML(virConnectPtr conn,
+                      const char *configDir,
+                      virNetworkDefPtr def,
+                      const char *xml)
 {
-    char *xml;
+    char *configFile = NULL;
     int fd = -1, ret = -1;
     size_t towrite;
     int err;
 
-    if (!net->configFile &&
-        virAsprintf(&net->configFile, "%s/%s.xml",
-                    configDir, net->def->name) < 0) {
-        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
-        goto cleanup;
-    }
-    if (!net->autostartLink &&
-        virAsprintf(&net->autostartLink, "%s/%s.xml",
-                    autostartDir, net->def->name) < 0) {
-        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
-        goto cleanup;
-    }
-
-    if (!(xml = virNetworkDefFormat(conn,
-                                    net->newDef ? net->newDef : net->def)))
+    if ((configFile = virNetworkConfigFile(conn, configDir, def->name)) == NULL)
         goto cleanup;
 
     if ((err = virFileMakePath(configDir))) {
@@ -675,19 +658,12 @@ int virNetworkSaveConfig(virConnectPtr c
         goto cleanup;
     }
 
-    if ((err = virFileMakePath(autostartDir))) {
-        virReportSystemError(conn, err,
-                             _("cannot create autostart directory '%s'"),
-                             autostartDir);
-        goto cleanup;
-    }
-
-    if ((fd = open(net->configFile,
+    if ((fd = open(configFile,
                    O_WRONLY | O_CREAT | O_TRUNC,
                    S_IRUSR | S_IWUSR )) < 0) {
         virReportSystemError(conn, errno,
                              _("cannot create config file '%s'"),
-                             net->configFile);
+                             configFile);
         goto cleanup;
     }
 
@@ -695,48 +671,63 @@ int virNetworkSaveConfig(virConnectPtr c
     if (safewrite(fd, xml, towrite) < 0) {
         virReportSystemError(conn, errno,
                              _("cannot write config file '%s'"),
-                             net->configFile);
+                             configFile);
         goto cleanup;
     }
 
     if (close(fd) < 0) {
         virReportSystemError(conn, errno,
                              _("cannot save config file '%s'"),
-                             net->configFile);
+                             configFile);
         goto cleanup;
     }
 
     ret = 0;
 
  cleanup:
-    VIR_FREE(xml);
     if (fd != -1)
         close(fd);
 
+    VIR_FREE(configFile);
+
     return ret;
 }
 
+int virNetworkSaveConfig(virConnectPtr conn,
+                         const char *configDir,
+                         virNetworkDefPtr def)
+{
+    int ret = -1;
+    char *xml;
+
+    if (!(xml = virNetworkDefFormat(conn, def)))
+        goto cleanup;
+
+    if (virNetworkSaveXML(conn, configDir, def, xml))
+        goto cleanup;
+
+    ret = 0;
+cleanup:
+    VIR_FREE(xml);
+    return ret;
+}
+
+
 virNetworkObjPtr virNetworkLoadConfig(virConnectPtr conn,
                                       virNetworkObjListPtr nets,
                                       const char *configDir,
                                       const char *autostartDir,
-                                      const char *file)
+                                      const char *name)
 {
     char *configFile = NULL, *autostartLink = NULL;
     virNetworkDefPtr def = NULL;
     virNetworkObjPtr net;
     int autostart;
 
-    if (virAsprintf(&configFile, "%s/%s",
-                    configDir, file) < 0) {
-        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
+    if ((configFile = virNetworkConfigFile(conn, configDir, name)) == NULL)
         goto error;
-    }
-    if (virAsprintf(&autostartLink, "%s/%s",
-                    autostartDir, file) < 0) {
-        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
+    if ((autostartLink = virNetworkConfigFile(conn, autostartDir, name)) == NULL)
         goto error;
-    }
 
     if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
         goto error;
@@ -744,7 +735,7 @@ virNetworkObjPtr virNetworkLoadConfig(vi
     if (!(def = virNetworkDefParseFile(conn, configFile)))
         goto error;
 
-    if (!virFileMatchesNameSuffix(file, def->name, ".xml")) {
+    if (!STREQ(name, def->name)) {
         virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
                               _("Network config filename '%s'"
                                 " does not match network name '%s'"),
@@ -755,10 +746,11 @@ virNetworkObjPtr virNetworkLoadConfig(vi
     if (!(net = virNetworkAssignDef(conn, nets, def)))
         goto error;
 
-    net->configFile = configFile;
-    net->autostartLink = autostartLink;
     net->autostart = autostart;
 
+    VIR_FREE(configFile);
+    VIR_FREE(autostartLink);
+
     return net;
 
 error:
@@ -791,7 +783,7 @@ int virNetworkLoadAllConfigs(virConnectP
         if (entry->d_name[0] == '.')
             continue;
 
-        if (!virFileHasSuffix(entry->d_name, ".xml"))
+        if (!virFileStripSuffix(entry->d_name, ".xml"))
             continue;
 
         /* NB: ignoring errors, so one malformed config doesn't
@@ -811,27 +803,51 @@ int virNetworkLoadAllConfigs(virConnectP
 }
 
 int virNetworkDeleteConfig(virConnectPtr conn,
+                           const char *configDir,
+                           const char *autostartDir,
                            virNetworkObjPtr net)
 {
-    if (!net->configFile || !net->autostartLink) {
-        virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
-                              _("no config file for %s"), net->def->name);
-        return -1;
-    }
+    char *configFile = NULL;
+    char *autostartLink = NULL;
+
+    if ((configFile = virNetworkConfigFile(conn, configDir, net->def->name)) == NULL)
+        goto error;
+    if ((autostartLink = virNetworkConfigFile(conn, autostartDir, net->def->name)) == NULL)
+        goto error;
 
     /* Not fatal if this doesn't work */
-    unlink(net->autostartLink);
+    unlink(autostartLink);
 
-    if (unlink(net->configFile) < 0) {
+    if (unlink(configFile) < 0) {
         virReportSystemError(conn, errno,
                              _("cannot remove config file '%s'"),
-                             net->configFile);
-        return -1;
+                             configFile);
+        goto error;
     }
 
     return 0;
+
+error:
+    VIR_FREE(configFile);
+    VIR_FREE(autostartLink);
+    return -1;
 }
 
+char *virNetworkConfigFile(virConnectPtr conn,
+                           const char *dir,
+                           const char *name)
+{
+    char *ret = NULL;
+
+    if (virAsprintf(&ret, "%s/%s.xml", dir, name) < 0) {
+        virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL);
+        return NULL;
+    }
+
+    return ret;
+}
+
+
 void virNetworkObjLock(virNetworkObjPtr obj)
 {
     virMutexLock(&obj->lock);
diff --git a/src/network_conf.h b/src/network_conf.h
--- a/src/network_conf.h
+++ b/src/network_conf.h
@@ -90,9 +90,6 @@ struct _virNetworkObj {
     unsigned int autostart : 1;
     unsigned int persistent : 1;
 
-    char *configFile;    /* Persistent config file path */
-    char *autostartLink; /* Symlink path for autostart */
-
     virNetworkDefPtr def; /* The current definition */
     virNetworkDefPtr newDef; /* New definition to activate at shutdown */
 };
@@ -139,10 +136,14 @@ char *virNetworkDefFormat(virConnectPtr 
                           const virNetworkDefPtr def);
 
 
+int virNetworkSaveXML(virConnectPtr conn,
+                      const char *configDir,
+                      virNetworkDefPtr def,
+                      const char *xml);
+
 int virNetworkSaveConfig(virConnectPtr conn,
                          const char *configDir,
-                         const char *autostartDir,
-                         virNetworkObjPtr net);
+                         virNetworkDefPtr def);
 
 virNetworkObjPtr virNetworkLoadConfig(virConnectPtr conn,
                                       virNetworkObjListPtr nets,
@@ -156,8 +157,15 @@ int virNetworkLoadAllConfigs(virConnectP
                              const char *autostartDir);
 
 int virNetworkDeleteConfig(virConnectPtr conn,
+                           const char *configDir,
+                           const char *autostartDir,
                            virNetworkObjPtr net);
 
+char *virNetworkConfigFile(virConnectPtr conn,
+                           const char *dir,
+                           const char *name);
+
+
 void virNetworkObjLock(virNetworkObjPtr obj);
 void virNetworkObjUnlock(virNetworkObjPtr obj);
 
diff --git a/src/network_driver.c b/src/network_driver.c
--- a/src/network_driver.c
+++ b/src/network_driver.c
@@ -57,6 +57,8 @@
 #include "iptables.h"
 #include "bridge.h"
 
+#define NETWORK_PID_DIR LOCAL_STATE_DIR "/run/libvirt/network"
+#define NETWORK_STATE_DIR LOCAL_STATE_DIR "/lib/libvirt/network"
 
 #define VIR_FROM_THIS VIR_FROM_NETWORK
 
@@ -106,6 +108,64 @@ static struct network_driver *driverStat
 
 
 static void
+networkFindActiveConfigs(struct network_driver *driver) {
+    unsigned int i;
+
+    for (i = 0 ; i < driver->networks.count ; i++) {
+        virNetworkObjPtr obj = driver->networks.objs[i];
+        virNetworkDefPtr tmp;
+        char *config;
+
+        virNetworkObjLock(obj);
+
+        if ((config = virNetworkConfigFile(NULL,
+                                           NETWORK_STATE_DIR,
+                                           obj->def->name)) == NULL) {
+            virNetworkObjUnlock(obj);
+            continue;
+        }
+
+        if (access(config, R_OK) < 0) {
+            VIR_FREE(config);
+            virNetworkObjUnlock(obj);
+            continue;
+        }
+
+        /* Try and load the live config */
+        tmp = virNetworkDefParseFile(NULL, config);
+        VIR_FREE(config);
+        if (tmp) {
+            obj->newDef = obj->def;
+            obj->def = tmp;
+        }
+
+        /* If bridge exists, then mark it active */
+        if (obj->def->bridge &&
+            brHasBridge(driver->brctl, obj->def->bridge) == 0) {
+            obj->active = 1;
+
+            /* Finally try and read dnsmasq pid if any DHCP ranges are set */
+            if (obj->def->nranges &&
+                virFileReadPid(NETWORK_PID_DIR, obj->def->name,
+                               &obj->dnsmasqPid) == 0) {
+
+                /* Check its still alive */
+                if (kill(obj->dnsmasqPid, 0) != 0)
+                    obj->dnsmasqPid = -1;
+
+                /* XXX ideally we'd check this was actually
+                 * the dnsmasq process, not a stale pid file
+                 * with someone else's process. But how ?
+                 */
+            }
+        }
+
+        virNetworkObjUnlock(obj);
+    }
+}
+
+
+static void
 networkAutostartConfigs(struct network_driver *driver) {
     unsigned int i;
 
@@ -132,6 +192,7 @@ static int
 networkStartup(void) {
     uid_t uid = geteuid();
     char *base = NULL;
+    int err;
 
     if (VIR_ALLOC(driverState) < 0)
         goto error;
@@ -181,12 +242,26 @@ networkStartup(void) {
 
     VIR_FREE(base);
 
+    if ((err = brInit(&driverState->brctl))) {
+        virReportSystemError(NULL, err, "%s",
+                             _("cannot initialize bridge support"));
+        goto error;
+    }
+
+    if (!(driverState->iptables = iptablesContextNew())) {
+        networkReportError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY,
+                           "%s", _("failed to allocate space for IP tables support"));
+        goto error;
+    }
+
+
     if (virNetworkLoadAllConfigs(NULL,
                                  &driverState->networks,
                                  driverState->networkConfigDir,
                                  driverState->networkAutostartDir) < 0)
         goto error;
 
+    networkFindActiveConfigs(driverState);
     networkAutostartConfigs(driverState);
 
     networkDriverUnlock(driverState);
@@ -269,23 +344,11 @@ networkActive(void) {
  */
 static int
 networkShutdown(void) {
-    unsigned int i;
-
     if (!driverState)
         return -1;
 
     networkDriverLock(driverState);
 
-    /* shutdown active networks */
-    for (i = 0 ; i < driverState->networks.count ; i++) {
-        virNetworkObjPtr net = driverState->networks.objs[i];
-        virNetworkObjLock(net);
-        if (virNetworkIsActive(net))
-            networkShutdownNetworkDaemon(NULL, driverState,
-                                         driverState->networks.objs[i]);
-        virNetworkObjUnlock(net);
-    }
-
     /* free inactive networks */
     virNetworkObjListFree(&driverState->networks);
 
@@ -309,23 +372,42 @@ networkShutdown(void) {
 
 static int
 networkBuildDnsmasqArgv(virConnectPtr conn,
-                      virNetworkObjPtr network,
-                      const char ***argv) {
+                        virNetworkObjPtr network,
+                        const char *pidfile,
+                        const char ***argv) {
     int i, len, r;
-    char buf[PATH_MAX];
+    char *pidfileArg;
+    char buf[1024];
+
+    /*
+     * NB, be careful about syntax for dnsmasq options in long format
+     *
+     * If the flag has a mandatory argument, it can be given using
+     * either syntax:
+     *
+     *     --foo bar
+     *     --foo=bar
+     *
+     * If the flag has a optional argument, it *must* be given using
+     * the syntax:
+     *
+     *     --foo=bar
+     *
+     * It is hard to determine whether a flag is optional or not,
+     * without reading the dnsmasq source :-( The manpages is not
+     * very explicit on this
+     */
 
     len =
         1 + /* dnsmasq */
-        1 + /* --keep-in-foreground */
         1 + /* --strict-order */
         1 + /* --bind-interfaces */
         (network->def->domain?2:0) + /* --domain name */
-        2 + /* --pid-file "" */
+        2 + /* --pid-file /var/run/libvirt/network/$NAME.pid */
         2 + /* --conf-file "" */
         /*2 + *//* --interface virbr0 */
         2 + /* --except-interface lo */
         2 + /* --listen-address 10.0.0.1 */
-        1 + /* --dhcp-leasefile=path */
         (2 * network->def->nranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 */
         /*  --dhcp-host 01:23:45:67:89:0a,hostname,10.0.0.3 */
         (2 * network->def->nhosts) +
@@ -339,11 +421,13 @@ networkBuildDnsmasqArgv(virConnectPtr co
             goto no_memory;          \
     } while (0)
 
+#define APPEND_ARG_LIT(v, n, s) \
+        (v)[(n)] = s
+
     i = 0;
 
     APPEND_ARG(*argv, i++, DNSMASQ);
 
-    APPEND_ARG(*argv, i++, "--keep-in-foreground");
     /*
      * Needed to ensure dnsmasq uses same algorithm for processing
      * multiple namedriver entries in /etc/resolv.conf as GLibC.
@@ -356,10 +440,11 @@ networkBuildDnsmasqArgv(virConnectPtr co
        APPEND_ARG(*argv, i++, network->def->domain);
     }
 
-    APPEND_ARG(*argv, i++, "--pid-file");
-    APPEND_ARG(*argv, i++, "");
+    if (virAsprintf(&pidfileArg, "--pid-file=%s", pidfile) < 0)
+        goto no_memory;
+    APPEND_ARG_LIT(*argv, i++, pidfileArg);
 
-    APPEND_ARG(*argv, i++, "--conf-file");
+    APPEND_ARG(*argv, i++, "--conf-file=");
     APPEND_ARG(*argv, i++, "");
 
     /*
@@ -377,15 +462,6 @@ networkBuildDnsmasqArgv(virConnectPtr co
     APPEND_ARG(*argv, i++, "--except-interface");
     APPEND_ARG(*argv, i++, "lo");
 
-    /*
-     * NB, dnsmasq command line arg bug means we need to
-     * use a single arg '--dhcp-leasefile=path' rather than
-     * two separate args in '--dhcp-leasefile path' style
-     */
-    snprintf(buf, sizeof(buf), "--dhcp-leasefile=%s/lib/libvirt/dhcp-%s.leases",
-             LOCAL_STATE_DIR, network->def->name);
-    APPEND_ARG(*argv, i++, buf);
-
     for (r = 0 ; r < network->def->nranges ; r++) {
         snprintf(buf, sizeof(buf), "%s,%s",
                  network->def->ranges[r].start,
@@ -434,7 +510,10 @@ dhcpStartDhcpDaemon(virConnectPtr conn,
                     virNetworkObjPtr network)
 {
     const char **argv;
-    int ret, i;
+    char *pidfile;
+    int ret = -1, i, err;
+
+    network->dnsmasqPid = -1;
 
     if (network->def->ipAddress == NULL) {
         networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
@@ -442,13 +521,49 @@ dhcpStartDhcpDaemon(virConnectPtr conn,
         return -1;
     }
 
+    if ((err = virFileMakePath(NETWORK_PID_DIR)) < 0) {
+        virReportSystemError(conn, err,
+                             _("cannot create directory %s"),
+                             NETWORK_PID_DIR);
+        return -1;
+    }
+    if ((err = virFileMakePath(NETWORK_STATE_DIR)) < 0) {
+        virReportSystemError(conn, err,
+                             _("cannot create directory %s"),
+                             NETWORK_STATE_DIR);
+        return -1;
+    }
+
+    if (!(pidfile = virFilePid(NETWORK_PID_DIR, network->def->name))) {
+        virReportOOMError(conn);
+        return -1;
+    }
+
     argv = NULL;
-    if (networkBuildDnsmasqArgv(conn, network, &argv) < 0)
+    if (networkBuildDnsmasqArgv(conn, network, pidfile, &argv) < 0) {
+        VIR_FREE(pidfile);
         return -1;
+    }
 
-    ret = virExec(conn, argv, NULL, NULL,
-                  &network->dnsmasqPid, -1, NULL, NULL, VIR_EXEC_NONBLOCK);
+    if (virRun(conn, argv, NULL) < 0)
+        goto cleanup;
 
+    /*
+     * There really is no race here - when dnsmasq daemonizes,
+     * its leader process stays around until its child has
+     * actually written its pidfile. So by time virRun exits
+     * it has waitpid'd and guaranteed the proess has started
+     * and written a pid
+     */
+
+    if (virFileReadPid(NETWORK_PID_DIR, network->def->name,
+                       &network->dnsmasqPid) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+cleanup:
+    VIR_FREE(pidfile);
     for (i = 0; argv[i]; i++)
         VIR_FREE(argv[i]);
     VIR_FREE(argv);
@@ -554,13 +669,6 @@ networkAddIptablesRules(virConnectPtr co
                       virNetworkObjPtr network) {
     int err;
 
-    if (!driver->iptables && !(driver->iptables = iptablesContextNew())) {
-        networkReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY,
-                     "%s", _("failed to allocate space for IP tables support"));
-        return 0;
-    }
-
-
     /* allow DHCP requests through to dnsmasq */
     if ((err = iptablesAddTcpInput(driver->iptables, network->def->bridge, 67))) {
         virReportSystemError(conn, err,
@@ -716,12 +824,6 @@ static int networkStartNetworkDaemon(vir
         return -1;
     }
 
-    if (!driver->brctl && (err = brInit(&driver->brctl))) {
-        virReportSystemError(conn, err, "%s",
-                             _("cannot initialize bridge support"));
-        return -1;
-    }
-
     if ((err = brAddBridge(driver->brctl, &network->def->bridge))) {
         virReportSystemError(conn, err,
                              _("cannot create bridge '%s'"),
@@ -729,7 +831,6 @@ static int networkStartNetworkDaemon(vir
         return -1;
     }
 
-
     if (brSetForwardDelay(driver->brctl, network->def->bridge, network->def->delay) < 0)
         goto err_delbr;
 
@@ -774,10 +875,22 @@ static int networkStartNetworkDaemon(vir
         dhcpStartDhcpDaemon(conn, network) < 0)
         goto err_delbr2;
 
+
+    /* Persist the live configuration now we have bridge info  */
+    if (virNetworkSaveConfig(conn, NETWORK_STATE_DIR, network->def) < 0) {
+        goto err_kill;
+    }
+
     network->active = 1;
 
     return 0;
 
+ err_kill:
+    if (network->dnsmasqPid > 0) {
+        kill(network->dnsmasqPid, SIGTERM);
+        network->dnsmasqPid = -1;
+    }
+
  err_delbr2:
     networkRemoveIptablesRules(driver, network);
 
@@ -798,16 +911,24 @@ static int networkStartNetworkDaemon(vir
 }
 
 
-static int networkShutdownNetworkDaemon(virConnectPtr conn ATTRIBUTE_UNUSED,
-                                      struct network_driver *driver,
-                                      virNetworkObjPtr network) {
+static int networkShutdownNetworkDaemon(virConnectPtr conn,
+                                        struct network_driver *driver,
+                                        virNetworkObjPtr network) {
     int err;
+    char *stateFile;
 
     networkLog(NETWORK_INFO, _("Shutting down network '%s'\n"), network->def->name);
 
     if (!virNetworkIsActive(network))
         return 0;
 
+    stateFile = virNetworkConfigFile(conn, NETWORK_STATE_DIR, network->def->name);
+    if (!stateFile)
+        return -1;
+
+    unlink(stateFile);
+    VIR_FREE(stateFile);
+
     if (network->dnsmasqPid > 0)
         kill(network->dnsmasqPid, SIGTERM);
 
@@ -824,13 +945,10 @@ static int networkShutdownNetworkDaemon(
                  network->def->bridge, strerror(err));
     }
 
+    /* See if its still alive and really really kill it */
     if (network->dnsmasqPid > 0 &&
-        waitpid(network->dnsmasqPid, NULL, WNOHANG) != network->dnsmasqPid) {
+        (kill(network->dnsmasqPid, 0) == 0))
         kill(network->dnsmasqPid, SIGKILL);
-        if (waitpid(network->dnsmasqPid, NULL, 0) != network->dnsmasqPid)
-            networkLog(NETWORK_WARN,
-                     "%s", _("Got unexpected pid for dnsmasq\n"));
-    }
 
     network->dnsmasqPid = -1;
     network->active = 0;
@@ -1048,8 +1166,7 @@ static virNetworkPtr networkDefine(virCo
 
     if (virNetworkSaveConfig(conn,
                              driver->networkConfigDir,
-                             driver->networkAutostartDir,
-                             network) < 0) {
+                             network->newDef ? network->newDef : network->def) < 0) {
         virNetworkRemoveInactive(&driver->networks,
                                  network);
         network = NULL;
@@ -1086,7 +1203,10 @@ static int networkUndefine(virNetworkPtr
         goto cleanup;
     }
 
-    if (virNetworkDeleteConfig(net->conn, network) < 0)
+    if (virNetworkDeleteConfig(net->conn,
+                               driver->networkConfigDir,
+                               driver->networkAutostartDir,
+                               network) < 0)
         goto cleanup;
 
     virNetworkRemoveInactive(&driver->networks,
@@ -1140,7 +1260,7 @@ static int networkDestroy(virNetworkPtr 
     }
 
     ret = networkShutdownNetworkDaemon(net->conn, driver, network);
-    if (!network->configFile) {
+    if (!network->persistent) {
         virNetworkRemoveInactive(&driver->networks,
                                  network);
         network = NULL;
@@ -1233,9 +1353,10 @@ cleanup:
 }
 
 static int networkSetAutostart(virNetworkPtr net,
-                             int autostart) {
+                               int autostart) {
     struct network_driver *driver = net->conn->networkPrivateData;
     virNetworkObjPtr network;
+    char *configFile = NULL, *autostartLink = NULL;
     int ret = -1;
 
     networkDriverLock(driver);
@@ -1251,6 +1372,11 @@ static int networkSetAutostart(virNetwor
     autostart = (autostart != 0);
 
     if (network->autostart != autostart) {
+        if ((configFile = virNetworkConfigFile(net->conn, driver->networkConfigDir, network->def->name)) == NULL)
+            goto cleanup;
+        if ((autostartLink = virNetworkConfigFile(net->conn, driver->networkAutostartDir, network->def->name)) == NULL)
+            goto cleanup;
+
         if (autostart) {
             int err;
 
@@ -1261,17 +1387,17 @@ static int networkSetAutostart(virNetwor
                 goto cleanup;
             }
 
-            if (symlink(network->configFile, network->autostartLink) < 0) {
+            if (symlink(configFile, autostartLink) < 0) {
                 virReportSystemError(net->conn, errno,
                                      _("Failed to create symlink '%s' to '%s'"),
-                                     network->autostartLink, network->configFile);
+                                     autostartLink, configFile);
                 goto cleanup;
             }
         } else {
-            if (unlink(network->autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) {
+            if (unlink(autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) {
                 virReportSystemError(net->conn, errno,
                                      _("Failed to delete symlink '%s'"),
-                                     network->autostartLink);
+                                     autostartLink);
                 goto cleanup;
             }
         }
@@ -1281,6 +1407,8 @@ static int networkSetAutostart(virNetwor
     ret = 0;
 
 cleanup:
+    VIR_FREE(configFile);
+    VIR_FREE(autostartLink);
     if (network)
         virNetworkObjUnlock(network);
     return ret;

-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]