2:49 p.m.
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.
THis patch depends on the 25 threading patches I sent earlier, so
you probably won't be able to apply it in isolation
Daniel
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, SIOCGIFMTU, &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,
@@ -58,6 +60,7 @@ int brDeleteInterface (brContr
const char *bridge,
const char *iface);
+
int brAddTap (brControl *ctl,
const char *bridge,
char **ifname,
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,64 @@ 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 +736,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 +747,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 +784,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 +804,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_LIB_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_LIB_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,23 @@ 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];
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 +402,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 +421,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 +443,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 +491,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 +502,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_LIB_DIR)) < 0) {
+ virReportSystemError(conn, err,
+ _("cannot create directory %s"),
+ NETWORK_LIB_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 guarenteed the proess has started
+ * and writtena 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 +650,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 +805,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 +812,6 @@ static int networkStartNetworkDaemon(vir
return -1;
}
-
if (brSetForwardDelay(driver->brctl, network->def->bridge,
network->def->delay) < 0)
goto err_delbr;
@@ -774,10 +856,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_LIB_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 +892,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 *configFile;
networkLog(NETWORK_INFO, _("Shutting down network '%s'\n"),
network->def->name);
if (!virNetworkIsActive(network))
return 0;
+ configFile = virNetworkConfigFile(conn, NETWORK_LIB_DIR, network->def->name);
+ if (!configFile)
+ return -1;
+
+ unlink(configFile);
+ VIR_FREE(configFile);
+
if (network->dnsmasqPid > 0)
kill(network->dnsmasqPid, SIGTERM);
@@ -824,13 +926,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 +1147,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 +1184,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 +1241,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 +1334,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 +1353,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 +1368,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 +1388,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 :|
2:55 a.m.
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.
...
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) {
^^
Two spaces! Oh noes! Yay for nitpicks!
+ 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, SIOCGIFMTU, &ifr))
+ return -1;
I'd probably use SIOCGIFFLAGS for this.
(Alternative is to use SIOCGIFBR to iterate over bridge names, but the
simpler approach is better)
...
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,
@@ -58,6 +60,7 @@ int brDeleteInterface (brContr
const char *bridge,
const char *iface);
+
Spurious.
...
@@ -695,48 +671,64 @@ 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)))
Odd formatting.
...
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_LIB_DIR LOCAL_STATE_DIR "/lib/libvirt/network"
s/LIB_DIR/STATE_DIR/ perhaps?
Also, I'd prefer to see dirs like this created by "make install" and/or
RPM install.
e.g. with read-only root if you wanted /var/lib/libvirt to be read-only
and bind-mount a tmpfs onto /var/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_LIB_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);
^
Missing whitespace.
...
@@ -377,15 +443,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
- */
Worth keeping this comment - e.g. someone could come along and change it
do s/=/ /, check that it works with newer dnsmasq and then 6 months
later get a report that it doesn't work with older dnsmasq.
- snprintf(buf, sizeof(buf),
"--dhcp-leasefile=%s/lib/libvirt/dhcp-%s.leases",
- LOCAL_STATE_DIR, network->def->name);
- APPEND_ARG(*argv, i++, buf);
--dhcp-leasefile not needed? Worth a comment in the commit log.
...
@@ -442,13 +502,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_LIB_DIR)) < 0) {
+ virReportSystemError(conn, err,
+ _("cannot create directory %s"),
+ NETWORK_LIB_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 guarenteed the proess has started
guaranteed
+ * and writtena pid
written a
...
@@ -798,16 +892,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 *configFile;
networkLog(NETWORK_INFO, _("Shutting down network '%s'\n"),
network->def->name);
if (!virNetworkIsActive(network))
return 0;
+ configFile = virNetworkConfigFile(conn, NETWORK_LIB_DIR, network->def->name);
+ if (!configFile)
+ return -1;
+
+ unlink(configFile);
+ VIR_FREE(configFile);
Perhaps rename this to stateFile - I got confused for a second thinking
you were deleting the actual config file here.
if (network->dnsmasqPid > 0)
kill(network->dnsmasqPid, SIGTERM);
@@ -824,13 +926,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"));
- }
Why no more waitpid? Zombies, no?
...
Cheers,
Mark.
5:10 a.m.
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.
> + 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, SIOCGIFMTU, &ifr))
> + return -1;
I'd probably use SIOCGIFFLAGS for this.
Yeah, sounds good to me - I just picked the first side-effect
free thing that I found. SIOCGIFFLAGS would let us check that
the interface was actually up, as well as merely existing.
>
> +int virNetworkSaveConfig(virConnectPtr conn,
> + const char *configDir,
> + virNetworkDefPtr def)
> +{
> + int ret = -1;
> + char *xml;
> +
> + if (!(xml = virNetworkDefFormat(conn,
> + def)))
Odd formatting.
Cut & paste from the virDomainSaveConfig, which had a number
of extra args to this equivalent method, hence multi-line.
> 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_LIB_DIR LOCAL_STATE_DIR "/lib/libvirt/network"
s/LIB_DIR/STATE_DIR/ perhaps?
Yes, would make more sense.
Also, I'd prefer to see dirs like this created by "make install" and/or
RPM install.
e.g. with read-only root if you wanted /var/lib/libvirt to be read-only
and bind-mount a tmpfs onto /var/lib/libvirt/network
Will sort that. SHould also pre-create the directories here used
by LXC / UML / QEMU drivers.
> @@ -377,15 +443,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
> - */
Worth keeping this comment - e.g. someone could come along and change it
do s/=/ /, check that it works with newer dnsmasq and then 6 months
later get a report that it doesn't work with older dnsmasq.
> - snprintf(buf, sizeof(buf),
"--dhcp-leasefile=%s/lib/libvirt/dhcp-%s.leases",
> - LOCAL_STATE_DIR, network->def->name);
> - APPEND_ARG(*argv, i++, buf);
--dhcp-leasefile not needed? Worth a comment in the commit log.
Turns out this support has been compiled out of the Fedora RPMs
for years, so its a silent no-op. Upstream has officially deprecated
its use and indicated that it will be removed, and if you've not
compiled out of the binary, its use will throw an error on startup.
It also isn't used for storing lease dnsmasq gives out - its only
for reading in a pre-defined set of mappings at startup. So basically
useless :-)
> +static int networkShutdownNetworkDaemon(virConnectPtr conn,
> + struct network_driver *driver,
> + virNetworkObjPtr network) {
> int err;
> + char *configFile;
>
> networkLog(NETWORK_INFO, _("Shutting down network '%s'\n"),
network->def->name);
>
> if (!virNetworkIsActive(network))
> return 0;
>
> + configFile = virNetworkConfigFile(conn, NETWORK_LIB_DIR,
network->def->name);
> + if (!configFile)
> + return -1;
> +
> + unlink(configFile);
> + VIR_FREE(configFile);
Perhaps rename this to stateFile - I got confused for a second thinking
you were deleting the actual config file here.
Ha, ok will rename that.
> if (network->dnsmasqPid > 0)
> kill(network->dnsmasqPid, SIGTERM);
>
> @@ -824,13 +926,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"));
> - }
Why no more waitpid? Zombies, no?
I removed the --keep-in-foreground flag from dnsmasq args, so the momnet
we start it, it daemonizes itself. We launch with virRun() instead of
virExec() which does the neccessary waitpid() at startup, so when shutting
it down, there's nothing to wait for.
Regards,
Daniel
--
|: 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 :|
6:44 a.m.
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 :|
9:02 a.m.
On Wed, Jan 14, 2009 at 12:44:50PM +0000, Daniel P. Berrange wrote:
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.
Sounds a real user improvement, patch looks fine but I'm concerned
about this:
+ /* 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 ?
+ */
OS specific but check in configure about the location of dnsmasq
export it as DNSMASQ_PATH, then compare to file pointed by /proc/pid/exe
#ifdef __linux__
char *pidpath;
virAsprintf(&pidpath, "/proc/%d/exe", obj->dnsmasqPid);
if (virFileLinkPointsTo(pidpath, DNSMASQ_PATH) == 0))
obj->dnsmasqPid = -1;
VIR_FREE(pidpath);
#endif
I'm sure one can find a Solaris equivalent.
Patch looks fine to me,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5826
days inactive
5829
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Mark McLoughlin