3:40 a.m.
Peter Krempa (2):
network: remove autostart flag from network when undefining it
network: bridge_driver: don't lose transient networks on daemon
restart
src/conf/network_conf.c | 220 +++++++++++++++++++++++++++++---------------
src/conf/network_conf.h | 10 +-
src/libvirt_private.syms | 2 +-
src/network/bridge_driver.c | 52 +++++++----
4 files changed, 185 insertions(+), 99 deletions(-)
--
1.8.1.5
3:40 a.m.
New subject: [libvirt] [PATCH 1/2] network: remove autostart flag from network when undefining it
When turning a started persistent network into a transient one we forgot
to remove the autostart flag that is no longer valid at that point.
---
src/network/bridge_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index e8b314a..bca6bd9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3193,6 +3193,7 @@ networkUndefine(virNetworkPtr net) {
/* make the network transient */
network->persistent = 0;
+ network->autostart = 0;
virNetworkDefFree(network->newDef);
network->newDef = NULL;
--
1.8.1.5
9:34 a.m.
New subject: [libvirt] [PATCH 1/2] network: remove autostart flag from network when undefining it
On 04/17/2013 04:40 AM, Peter Krempa wrote:
When turning a started persistent network into a transient one we
forgot
to remove the autostart flag that is no longer valid at that point.
---
src/network/bridge_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index e8b314a..bca6bd9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3193,6 +3193,7 @@ networkUndefine(virNetworkPtr net) {
/* make the network transient */
network->persistent = 0;
+ network->autostart = 0;
virNetworkDefFree(network->newDef);
network->newDef = NULL;
ACK.
3:20 a.m.
New subject: [libvirt] [PATCH 1/2] network: remove autostart flag from network when undefining it
On 04/17/13 16:34, Laine Stump wrote:
On 04/17/2013 04:40 AM, Peter Krempa wrote:
> When turning a started persistent network into a transient one we forgot
> to remove the autostart flag that is no longer valid at that point.
> ---
> src/network/bridge_driver.c | 1 +
> 1 file changed, 1 insertion(+)
>
ACK.
Pushed. Thanks.
Peter
3:40 a.m.
New subject: [libvirt] [PATCH 2/2] network: bridge_driver: don't lose transient networks on daemon restart
Until now tranisent networks weren't really useful as libvirtd wasn't
able to remember them across restarts. This patch adds support for
loading status files of transient networks (that already were generated)
so that the status isn't lost.
This patch chops up virNetworkObjUpdateParseFile and turns it into
virNetworkLoadState and a few friends that will help us to load status
XMLs and refactors the functions that are loading the configs to use
them.
---
src/conf/network_conf.c | 220 +++++++++++++++++++++++++++++---------------
src/conf/network_conf.h | 10 +-
src/libvirt_private.syms | 2 +-
src/network/bridge_driver.c | 51 ++++++----
4 files changed, 184 insertions(+), 99 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 75584a0..009e90c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1967,81 +1967,6 @@ cleanup:
return def;
}
-int
-virNetworkObjUpdateParseFile(const char *filename,
- virNetworkObjPtr net)
-{
- int ret = -1;
- xmlDocPtr xml = NULL;
- xmlNodePtr node = NULL;
- virNetworkDefPtr tmp = NULL;
- xmlXPathContextPtr ctxt = NULL;
-
- xml = virXMLParse(filename, NULL, _("(network status)"));
- if (!xml)
- return -1;
-
- ctxt = xmlXPathNewContext(xml);
- if (ctxt == NULL) {
- virReportOOMError();
- goto cleanup;
- }
-
- node = xmlDocGetRootElement(xml);
- if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
- /* Newer network status file. Contains useful
- * info which are not to be found in bare config XML */
- char *class_id = NULL;
- char *floor_sum = NULL;
-
- ctxt->node = node;
- class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt);
- if (class_id) {
- virBitmapFree(net->class_id);
- if (virBitmapParse(class_id, 0,
- &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Malformed 'class_id' attribute:
%s"),
- class_id);
- VIR_FREE(class_id);
- goto cleanup;
- }
- }
- VIR_FREE(class_id);
-
- floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
- if (floor_sum &&
- virStrToLong_ull(floor_sum, NULL, 10, &net->floor_sum) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Malformed 'floor_sum' attribute: %s"),
- floor_sum);
- VIR_FREE(floor_sum);
- }
- VIR_FREE(floor_sum);
- }
-
- node = virXPathNode("//network", ctxt);
- if (!node) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Could not find any 'network' element"));
- goto cleanup;
- }
-
- ctxt->node = node;
- tmp = virNetworkDefParseXML(ctxt);
-
- if (tmp) {
- net->newDef = net->def;
- net->def = tmp;
- }
-
- ret = 0;
-
-cleanup:
- xmlFreeDoc(xml);
- xmlXPathFreeContext(ctxt);
- return ret;
-}
static int
virNetworkDNSDefFormat(virBufferPtr buf,
@@ -2554,6 +2479,117 @@ cleanup:
return ret;
}
+virNetworkObjPtr
+virNetworkLoadState(virNetworkObjListPtr nets,
+ const char *configDir,
+ const char *name)
+{
+ char *configFile = NULL;
+ virNetworkDefPtr def = NULL;
+ virNetworkObjPtr net = NULL;
+ xmlDocPtr xml = NULL;
+ xmlNodePtr node = NULL;
+ xmlXPathContextPtr ctxt = NULL;
+ virBitmapPtr class_id_map = NULL;
+ unsigned long long floor_sum_val = 0;
+
+
+ if ((configFile = virNetworkConfigFile(configDir, name)) == NULL)
+ goto error;
+
+ if (!(xml = virXMLParseCtxt(configFile, NULL, _("(network status)"),
&ctxt)))
+ goto error;
+
+ if (!(node = virXPathNode("//network", ctxt))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not find any 'network' element in status
file"));
+ goto error;
+ }
+
+ /* parse the definition first */
+ ctxt->node = node;
+ if (!(def = virNetworkDefParseXML(ctxt)))
+ goto error;
+
+ if (!STREQ(name, def->name)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Network config filename '%s'"
+ " does not match network name '%s'"),
+ configFile, def->name);
+ goto error;
+ }
+
+ if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
+ def->forward.type == VIR_NETWORK_FORWARD_NAT ||
+ def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
+ /* state configs should always have the bridge stored in the file */
+ if (!def->bridge || strstr(def->bridge, "%d")) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("network status XML does not contain valid bridge
"
+ "name"));
+ goto error;
+ }
+ }
+
+ /* now parse possible status data */
+ node = xmlDocGetRootElement(xml);
+ if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
+ /* Newer network status file. Contains useful
+ * info which are not to be found in bare config XML */
+ char *class_id = NULL;
+ char *floor_sum = NULL;
+
+ ctxt->node = node;
+ if ((class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt)))
{
+ if (virBitmapParse(class_id, 0, &class_id_map,
+ CLASS_ID_BITMAP_SIZE) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Malformed 'class_id' attribute:
%s"),
+ class_id);
+ VIR_FREE(class_id);
+ goto error;
+ }
+ }
+ VIR_FREE(class_id);
+
+ floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
+ if (floor_sum &&
+ virStrToLong_ull(floor_sum, NULL, 10, &floor_sum_val) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Malformed 'floor_sum' attribute: %s"),
+ floor_sum);
+ VIR_FREE(floor_sum);
+ }
+ VIR_FREE(floor_sum);
+ }
+
+ /* create the object */
+ if (!(net = virNetworkAssignDef(nets, def, true)))
+ goto error;
+ /* no goto error below this comment */
+
+ /* assign status data stored in the network object */
+ if (class_id_map) {
+ virBitmapFree(net->class_id);
+ net->class_id = class_id_map;
+ }
+
+ if (floor_sum_val > 0)
+ net->floor_sum = floor_sum_val;
+
+
+cleanup:
+ VIR_FREE(configFile);
+ xmlFreeDoc(xml);
+ xmlXPathFreeContext(ctxt);
+ return net;
+
+error:
+ virBitmapFree(class_id_map);
+ virNetworkDefFree(def);
+ goto cleanup;
+}
+
virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
const char *configDir,
const char *autostartDir,
@@ -2612,6 +2648,40 @@ error:
return NULL;
}
+
+int
+virNetworkLoadAllState(virNetworkObjListPtr nets,
+ const char *configDir)
+{
+ DIR *dir;
+ struct dirent *entry;
+
+ if (!(dir = opendir(configDir))) {
+ if (errno == ENOENT)
+ return 0;
+
+ virReportSystemError(errno, _("Failed to open dir '%s'"),
configDir);
+ return -1;
+ }
+
+ while ((entry = readdir(dir))) {
+ virNetworkObjPtr net;
+
+ if (entry->d_name[0] == '.')
+ continue;
+
+ if (!virFileStripSuffix(entry->d_name, ".xml"))
+ continue;
+
+ if ((net = virNetworkLoadState(nets, configDir, entry->d_name)))
+ virNetworkObjUnlock(net);
+ }
+
+ closedir(dir);
+ return 0;
+}
+
+
int virNetworkLoadAllConfigs(virNetworkObjListPtr nets,
const char *configDir,
const char *autostartDir)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 1a86e3d..7d4c40b 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -280,9 +280,6 @@ virNetworkDefPtr virNetworkDefParseString(const char *xmlStr);
virNetworkDefPtr virNetworkDefParseFile(const char *filename);
virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml,
xmlNodePtr root);
-int virNetworkObjUpdateParseFile(const char *filename,
- virNetworkObjPtr net);
-
char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags);
static inline const char *
@@ -318,10 +315,17 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
const char *autostartDir,
const char *file);
+virNetworkObjPtr virNetworkLoadState(virNetworkObjListPtr nets,
+ const char *configDir,
+ const char *name);
+
int virNetworkLoadAllConfigs(virNetworkObjListPtr nets,
const char *configDir,
const char *autostartDir);
+int virNetworkLoadAllState(virNetworkObjListPtr nets,
+ const char *configDir);
+
int virNetworkDeleteConfig(const char *configDir,
const char *autostartDir,
virNetworkObjPtr net);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 30fdcd7..f778e9c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -454,6 +454,7 @@ virNetworkIpDefNetmask;
virNetworkIpDefPrefix;
virNetworkList;
virNetworkLoadAllConfigs;
+virNetworkLoadAllState;
virNetworkObjAssignDef;
virNetworkObjFree;
virNetworkObjGetPersistentDef;
@@ -465,7 +466,6 @@ virNetworkObjSetDefTransient;
virNetworkObjUnlock;
virNetworkObjUnsetDefTransient;
virNetworkObjUpdate;
-virNetworkObjUpdateParseFile;
virNetworkRemoveInactive;
virNetworkSaveConfig;
virNetworkSaveStatus;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index bca6bd9..56af108 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -181,6 +181,7 @@ networkRemoveInactive(struct network_driver *driver,
char *radvdconfigfile = NULL;
char *configfile = NULL;
char *radvdpidbase = NULL;
+ char *statusfile = NULL;
dnsmasqContext *dctx = NULL;
virNetworkDefPtr def = virNetworkObjGetPersistentDef(net);
@@ -202,6 +203,9 @@ networkRemoveInactive(struct network_driver *driver,
if (!(configfile = networkDnsmasqConfigFileName(def->name)))
goto no_memory;
+ if (!(statusfile = virNetworkConfigFile(NETWORK_STATE_DIR, def->name)))
+ goto no_memory;
+
/* dnsmasq */
dnsmasqDelete(dctx);
unlink(leasefile);
@@ -211,6 +215,9 @@ networkRemoveInactive(struct network_driver *driver,
unlink(radvdconfigfile);
virPidFileDelete(NETWORK_PID_DIR, radvdpidbase);
+ /* remove status file */
+ unlink(statusfile);
+
/* remove the network definition */
virNetworkRemoveInactive(&driver->networks, net);
@@ -221,6 +228,7 @@ cleanup:
VIR_FREE(configfile);
VIR_FREE(radvdconfigfile);
VIR_FREE(radvdpidbase);
+ VIR_FREE(statusfile);
dnsmasqContextFree(dctx);
return ret;
@@ -254,33 +262,15 @@ networkBridgeDummyNicName(const char *brname)
}
static void
-networkFindActiveConfigs(struct network_driver *driver) {
+networkFindActiveConfigs(struct network_driver *driver)
+{
unsigned int i;
for (i = 0 ; i < driver->networks.count ; i++) {
virNetworkObjPtr obj = driver->networks.objs[i];
- char *config;
virNetworkObjLock(obj);
- if ((config = virNetworkConfigFile(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 */
- if (virNetworkObjUpdateParseFile(config, obj) < 0)
- VIR_WARN("Unable to update config of '%s' network",
- obj->def->name);
- VIR_FREE(config);
-
/* If bridge exists, then mark it active */
if (obj->def->bridge &&
virNetDevExists(obj->def->bridge) == 1) {
@@ -307,6 +297,21 @@ networkFindActiveConfigs(struct network_driver *driver) {
cleanup:
virNetworkObjUnlock(obj);
}
+
+redo:
+ /* remove inactive transient domains */
+ for (i = 0 ; i < driver->networks.count ; i++) {
+ virNetworkObjPtr obj = driver->networks.objs[i];
+
+ virNetworkObjLock(obj);
+
+ if (!obj->persistent && !obj->active) {
+ networkRemoveInactive(driver, obj);
+ goto redo;
+ }
+
+ virNetworkObjUnlock(obj);
+ }
}
@@ -416,6 +421,10 @@ networkStartup(bool privileged,
/* if this fails now, it will be retried later with dnsmasqCapsRefresh() */
driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
+ if (virNetworkLoadAllState(&driverState->networks,
+ NETWORK_STATE_DIR) < 0)
+ goto error;
+
if (virNetworkLoadAllConfigs(&driverState->networks,
driverState->networkConfigDir,
driverState->networkAutostartDir) < 0)
@@ -480,6 +489,8 @@ networkReload(void) {
return 0;
networkDriverLock(driverState);
+ virNetworkLoadAllState(&driverState->networks,
+ NETWORK_STATE_DIR);
virNetworkLoadAllConfigs(&driverState->networks,
driverState->networkConfigDir,
driverState->networkAutostartDir);
--
1.8.1.5
11:07 a.m.
New subject: [libvirt] [PATCH 2/2] network: bridge_driver: don't lose transient networks on daemon restart
On 04/17/2013 04:40 AM, Peter Krempa wrote:
Until now tranisent networks weren't really useful as libvirtd
wasn't
able to remember them across restarts. This patch adds support for
loading status files of transient networks (that already were generated)
so that the status isn't lost.
This patch chops up virNetworkObjUpdateParseFile and turns it into
virNetworkLoadState and a few friends that will help us to load status
XMLs and refactors the functions that are loading the configs to use
them.
---
src/conf/network_conf.c | 220 +++++++++++++++++++++++++++++---------------
src/conf/network_conf.h | 10 +-
src/libvirt_private.syms | 2 +-
src/network/bridge_driver.c | 51 ++++++----
4 files changed, 184 insertions(+), 99 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 75584a0..009e90c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1967,81 +1967,6 @@ cleanup:
return def;
}
-int
-virNetworkObjUpdateParseFile(const char *filename,
- virNetworkObjPtr net)
-{
- int ret = -1;
- xmlDocPtr xml = NULL;
- xmlNodePtr node = NULL;
- virNetworkDefPtr tmp = NULL;
- xmlXPathContextPtr ctxt = NULL;
-
- xml = virXMLParse(filename, NULL, _("(network status)"));
- if (!xml)
- return -1;
-
- ctxt = xmlXPathNewContext(xml);
- if (ctxt == NULL) {
- virReportOOMError();
- goto cleanup;
- }
-
- node = xmlDocGetRootElement(xml);
- if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
- /* Newer network status file. Contains useful
- * info which are not to be found in bare config XML */
- char *class_id = NULL;
- char *floor_sum = NULL;
-
- ctxt->node = node;
- class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt);
- if (class_id) {
- virBitmapFree(net->class_id);
- if (virBitmapParse(class_id, 0,
- &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Malformed 'class_id' attribute:
%s"),
- class_id);
- VIR_FREE(class_id);
- goto cleanup;
- }
- }
- VIR_FREE(class_id);
-
- floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
- if (floor_sum &&
- virStrToLong_ull(floor_sum, NULL, 10, &net->floor_sum) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Malformed 'floor_sum' attribute: %s"),
- floor_sum);
- VIR_FREE(floor_sum);
- }
- VIR_FREE(floor_sum);
- }
-
- node = virXPathNode("//network", ctxt);
- if (!node) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Could not find any 'network' element"));
- goto cleanup;
- }
-
- ctxt->node = node;
- tmp = virNetworkDefParseXML(ctxt);
-
- if (tmp) {
- net->newDef = net->def;
- net->def = tmp;
- }
-
- ret = 0;
-
-cleanup:
- xmlFreeDoc(xml);
- xmlXPathFreeContext(ctxt);
- return ret;
-}
static int
virNetworkDNSDefFormat(virBufferPtr buf,
@@ -2554,6 +2479,117 @@ cleanup:
return ret;
}
+virNetworkObjPtr
+virNetworkLoadState(virNetworkObjListPtr nets,
+ const char *configDir,
Maybe call it "stateDir" just to avoid confusion?
+ const char *name)
+{
+ char *configFile = NULL;
+ virNetworkDefPtr def = NULL;
+ virNetworkObjPtr net = NULL;
+ xmlDocPtr xml = NULL;
+ xmlNodePtr node = NULL;
+ xmlXPathContextPtr ctxt = NULL;
+ virBitmapPtr class_id_map = NULL;
+ unsigned long long floor_sum_val = 0;
+
+
+ if ((configFile = virNetworkConfigFile(configDir, name)) == NULL)
+ goto error;
+
+ if (!(xml = virXMLParseCtxt(configFile, NULL, _("(network status)"),
&ctxt)))
+ goto error;
+
+ if (!(node = virXPathNode("//network", ctxt))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not find any 'network' element in status
file"));
+ goto error;
+ }
Won't that fail when reading an old-style network status file that has
<network> at the toplevel rather than inside <networkstatus>? Or is
"//xyz" some special XPath notation that means "find this node anywhere
in the hierarchy"?
+
+ /* parse the definition first */
+ ctxt->node = node;
+ if (!(def = virNetworkDefParseXML(ctxt)))
+ goto error;
+
+ if (!STREQ(name, def->name)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Network config filename '%s'"
+ " does not match network name '%s'"),
+ configFile, def->name);
+ goto error;
+ }
+
+ if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
+ def->forward.type == VIR_NETWORK_FORWARD_NAT ||
+ def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
+ /* state configs should always have the bridge stored in the file */
+ if (!def->bridge || strstr(def->bridge, "%d")) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("network status XML does not contain valid bridge
"
+ "name"));
+ goto error;
+ }
+ }
Is there a reason this extra validation was needed here? We don't use
def->bridge anywhere else in this function, and I would assume that the
other functions using def after return will be properly validating what
they use.
+
+ /* now parse possible status data */
+ node = xmlDocGetRootElement(xml);
+ if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
+ /* Newer network status file. Contains useful
+ * info which are not to be found in bare config XML */
+ char *class_id = NULL;
+ char *floor_sum = NULL;
+
+ ctxt->node = node;
+ if ((class_id = virXPathString("string(./class_id[1]/@bitmap)",
ctxt))) {
+ if (virBitmapParse(class_id, 0, &class_id_map,
+ CLASS_ID_BITMAP_SIZE) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Malformed 'class_id' attribute:
%s"),
+ class_id);
+ VIR_FREE(class_id);
+ goto error;
+ }
+ }
+ VIR_FREE(class_id);
+
+ floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
+ if (floor_sum &&
+ virStrToLong_ull(floor_sum, NULL, 10, &floor_sum_val) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Malformed 'floor_sum' attribute: %s"),
+ floor_sum);
+ VIR_FREE(floor_sum);
+ }
+ VIR_FREE(floor_sum);
+ }
+
+ /* create the object */
+ if (!(net = virNetworkAssignDef(nets, def, true)))
+ goto error;
+ /* no goto error below this comment */
You should make that comment less ambiguous, i.e. "do not put any "goto
error" anywhere below this comment"
+
+ /* assign status data stored in the network object */
+ if (class_id_map) {
+ virBitmapFree(net->class_id);
Rather than calling virBitmapFree here, I think it would be better to
call it after cleanup: (and remove it from error:), just to make it
easier for someone to verify that it will always be properly freed.
+ net->class_id = class_id_map;
+ }
+
+ if (floor_sum_val > 0)
+ net->floor_sum = floor_sum_val;
+
+
+cleanup:
+ VIR_FREE(configFile);
+ xmlFreeDoc(xml);
+ xmlXPathFreeContext(ctxt);
+ return net;
+
+error:
+ virBitmapFree(class_id_map);
+ virNetworkDefFree(def);
+ goto cleanup;
+}
+
virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
const char *configDir,
const char *autostartDir,
@@ -2612,6 +2648,40 @@ error:
return NULL;
}
+
+int
+virNetworkLoadAllState(virNetworkObjListPtr nets,
+ const char *configDir)
+{
+ DIR *dir;
+ struct dirent *entry;
+
+ if (!(dir = opendir(configDir))) {
+ if (errno == ENOENT)
+ return 0;
+
+ virReportSystemError(errno, _("Failed to open dir '%s'"),
configDir);
+ return -1;
+ }
+
+ while ((entry = readdir(dir))) {
+ virNetworkObjPtr net;
+
+ if (entry->d_name[0] == '.')
+ continue;
+
+ if (!virFileStripSuffix(entry->d_name, ".xml"))
+ continue;
+
+ if ((net = virNetworkLoadState(nets, configDir, entry->d_name)))
+ virNetworkObjUnlock(net);
+ }
+
+ closedir(dir);
+ return 0;
+}
+
+
int virNetworkLoadAllConfigs(virNetworkObjListPtr nets,
const char *configDir,
const char *autostartDir)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 1a86e3d..7d4c40b 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -280,9 +280,6 @@ virNetworkDefPtr virNetworkDefParseString(const char *xmlStr);
virNetworkDefPtr virNetworkDefParseFile(const char *filename);
virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml,
xmlNodePtr root);
-int virNetworkObjUpdateParseFile(const char *filename,
- virNetworkObjPtr net);
-
char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags);
static inline const char *
@@ -318,10 +315,17 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
const char *autostartDir,
const char *file);
+virNetworkObjPtr virNetworkLoadState(virNetworkObjListPtr nets,
+ const char *configDir,
+ const char *name);
+
int virNetworkLoadAllConfigs(virNetworkObjListPtr nets,
const char *configDir,
const char *autostartDir);
+int virNetworkLoadAllState(virNetworkObjListPtr nets,
+ const char *configDir);
+
int virNetworkDeleteConfig(const char *configDir,
const char *autostartDir,
virNetworkObjPtr net);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 30fdcd7..f778e9c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -454,6 +454,7 @@ virNetworkIpDefNetmask;
virNetworkIpDefPrefix;
virNetworkList;
virNetworkLoadAllConfigs;
+virNetworkLoadAllState;
virNetworkObjAssignDef;
virNetworkObjFree;
virNetworkObjGetPersistentDef;
@@ -465,7 +466,6 @@ virNetworkObjSetDefTransient;
virNetworkObjUnlock;
virNetworkObjUnsetDefTransient;
virNetworkObjUpdate;
-virNetworkObjUpdateParseFile;
virNetworkRemoveInactive;
virNetworkSaveConfig;
virNetworkSaveStatus;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index bca6bd9..56af108 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -181,6 +181,7 @@ networkRemoveInactive(struct network_driver *driver,
char *radvdconfigfile = NULL;
char *configfile = NULL;
char *radvdpidbase = NULL;
+ char *statusfile = NULL;
dnsmasqContext *dctx = NULL;
virNetworkDefPtr def = virNetworkObjGetPersistentDef(net);
@@ -202,6 +203,9 @@ networkRemoveInactive(struct network_driver *driver,
if (!(configfile = networkDnsmasqConfigFileName(def->name)))
goto no_memory;
+ if (!(statusfile = virNetworkConfigFile(NETWORK_STATE_DIR, def->name)))
+ goto no_memory;
+
/* dnsmasq */
dnsmasqDelete(dctx);
unlink(leasefile);
@@ -211,6 +215,9 @@ networkRemoveInactive(struct network_driver *driver,
unlink(radvdconfigfile);
virPidFileDelete(NETWORK_PID_DIR, radvdpidbase);
+ /* remove status file */
+ unlink(statusfile);
+
/* remove the network definition */
virNetworkRemoveInactive(&driver->networks, net);
@@ -221,6 +228,7 @@ cleanup:
VIR_FREE(configfile);
VIR_FREE(radvdconfigfile);
VIR_FREE(radvdpidbase);
+ VIR_FREE(statusfile);
dnsmasqContextFree(dctx);
return ret;
@@ -254,33 +262,15 @@ networkBridgeDummyNicName(const char *brname)
}
static void
-networkFindActiveConfigs(struct network_driver *driver) {
+networkFindActiveConfigs(struct network_driver *driver)
+{
unsigned int i;
for (i = 0 ; i < driver->networks.count ; i++) {
virNetworkObjPtr obj = driver->networks.objs[i];
- char *config;
virNetworkObjLock(obj);
- if ((config = virNetworkConfigFile(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 */
- if (virNetworkObjUpdateParseFile(config, obj) < 0)
- VIR_WARN("Unable to update config of '%s' network",
- obj->def->name);
- VIR_FREE(config);
-
/* If bridge exists, then mark it active */
if (obj->def->bridge &&
virNetDevExists(obj->def->bridge) == 1) {
This simple check for the bridge device's existence isn't adequate for
network types that have no bridge, or for network types that have a
bridge that isn't managed by libvirt (i.e. forward
mode='bridge|hostdev'). In those cases libvirt makes no external changes
to the system that could be used to detect if the network is active or
not - the only useful indicator is the presence of the state file. (In
general I think that should be true for networks where libvirt
creates/manages the bridge too; if the state file exists but the bridge
doesn't, that just means that the network needs to be reconstructed, not
that it should be marked inactive).
@@ -307,6 +297,21 @@ networkFindActiveConfigs(struct network_driver
*driver) {
cleanup:
virNetworkObjUnlock(obj);
}
+
+redo:
+ /* remove inactive transient domains */
+ for (i = 0 ; i < driver->networks.count ; i++) {
+ virNetworkObjPtr obj = driver->networks.objs[i];
+
+ virNetworkObjLock(obj);
+
+ if (!obj->persistent && !obj->active) {
+ networkRemoveInactive(driver, obj);
I think you have a deadlock here, don't you? networkRemoveInactive()
calls virNetworkRemoveInactive(), which will scan through this same
array, locking each network object before seeing if it's the one we want
to delete. But you've already locked it.
Since we already hold the network driver lock, and we're operating at a
single-threaded place for the network driver anyway, I think the
virNetworkObjLock() here is unnecessary.
+ goto redo;
It seems a bit drastic to redo all the way from the beginning of the
list. Since this function already assumes enough knowledge about the
list of network objects to know that it's an array, and it knows which
position in the array contains the object we're deleting, we might as
well go all the way and let this code "know" that when an object is
removed, the remainder of the list is moved up by one; then all we need
to do is go to the top of the loop without doing i++.
So, you could just write this as a while loop instead of a for loop, put
the "i++;" at the bottom of the loop, and do a "continue;" instead of
"goto redo;"
+ }
+
+ virNetworkObjUnlock(obj);
+ }
}
@@ -416,6 +421,10 @@ networkStartup(bool privileged,
/* if this fails now, it will be retried later with dnsmasqCapsRefresh() */
driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
+ if (virNetworkLoadAllState(&driverState->networks,
+ NETWORK_STATE_DIR) < 0)
+ goto error;
+
if (virNetworkLoadAllConfigs(&driverState->networks,
driverState->networkConfigDir,
driverState->networkAutostartDir) < 0)
@@ -480,6 +489,8 @@ networkReload(void) {
return 0;
networkDriverLock(driverState);
+ virNetworkLoadAllState(&driverState->networks,
+ NETWORK_STATE_DIR);
virNetworkLoadAllConfigs(&driverState->networks,
driverState->networkConfigDir,
driverState->networkAutostartDir);
Otherwise looks good, and will be a nice thing to get fixed.
7:10 a.m.
New subject: [libvirt] [PATCH 2/2] network: bridge_driver: don't lose transient networks on daemon restart
On 04/17/13 18:07, Laine Stump wrote:
On 04/17/2013 04:40 AM, Peter Krempa wrote:
> Until now tranisent networks weren't really useful as libvirtd wasn't
> able to remember them across restarts. This patch adds support for
> loading status files of transient networks (that already were generated)
> so that the status isn't lost.
>
> This patch chops up virNetworkObjUpdateParseFile and turns it into
> virNetworkLoadState and a few friends that will help us to load status
> XMLs and refactors the functions that are loading the configs to use
> them.
> ---
> src/conf/network_conf.c | 220 +++++++++++++++++++++++++++++---------------
> src/conf/network_conf.h | 10 +-
> src/libvirt_private.syms | 2 +-
> src/network/bridge_driver.c | 51 ++++++----
> 4 files changed, 184 insertions(+), 99 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 75584a0..009e90c 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1967,81 +1967,6 @@ cleanup:
> return def;
> }
>
> -int
> -virNetworkObjUpdateParseFile(const char *filename,
> - virNetworkObjPtr net)
> -{
> - int ret = -1;
> - xmlDocPtr xml = NULL;
> - xmlNodePtr node = NULL;
> - virNetworkDefPtr tmp = NULL;
> - xmlXPathContextPtr ctxt = NULL;
> -
> - xml = virXMLParse(filename, NULL, _("(network status)"));
> - if (!xml)
> - return -1;
> -
> - ctxt = xmlXPathNewContext(xml);
> - if (ctxt == NULL) {
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> - node = xmlDocGetRootElement(xml);
> - if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
> - /* Newer network status file. Contains useful
> - * info which are not to be found in bare config XML */
> - char *class_id = NULL;
> - char *floor_sum = NULL;
> -
> - ctxt->node = node;
> - class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt);
> - if (class_id) {
> - virBitmapFree(net->class_id);
> - if (virBitmapParse(class_id, 0,
> - &net->class_id, CLASS_ID_BITMAP_SIZE) < 0)
{
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Malformed 'class_id' attribute:
%s"),
> - class_id);
> - VIR_FREE(class_id);
> - goto cleanup;
> - }
> - }
> - VIR_FREE(class_id);
> -
> - floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
> - if (floor_sum &&
> - virStrToLong_ull(floor_sum, NULL, 10, &net->floor_sum) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Malformed 'floor_sum' attribute:
%s"),
> - floor_sum);
> - VIR_FREE(floor_sum);
> - }
> - VIR_FREE(floor_sum);
> - }
> -
> - node = virXPathNode("//network", ctxt);
> - if (!node) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Could not find any 'network'
element"));
> - goto cleanup;
> - }
> -
> - ctxt->node = node;
> - tmp = virNetworkDefParseXML(ctxt);
> -
> - if (tmp) {
> - net->newDef = net->def;
> - net->def = tmp;
> - }
> -
> - ret = 0;
> -
> -cleanup:
> - xmlFreeDoc(xml);
> - xmlXPathFreeContext(ctxt);
> - return ret;
> -}
>
> static int
> virNetworkDNSDefFormat(virBufferPtr buf,
> @@ -2554,6 +2479,117 @@ cleanup:
> return ret;
> }
>
> +virNetworkObjPtr
> +virNetworkLoadState(virNetworkObjListPtr nets,
> + const char *configDir,
Maybe call it "stateDir" just to avoid confusion?
That sounds better.
> + const char *name)
> +{
> + char *configFile = NULL;
> + virNetworkDefPtr def = NULL;
> + virNetworkObjPtr net = NULL;
> + xmlDocPtr xml = NULL;
> + xmlNodePtr node = NULL;
> + xmlXPathContextPtr ctxt = NULL;
> + virBitmapPtr class_id_map = NULL;
> + unsigned long long floor_sum_val = 0;
> +
> +
> + if ((configFile = virNetworkConfigFile(configDir, name)) == NULL)
> + goto error;
> +
> + if (!(xml = virXMLParseCtxt(configFile, NULL, _("(network status)"),
&ctxt)))
> + goto error;
> +
> + if (!(node = virXPathNode("//network", ctxt))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not find any 'network' element in
status file"));
> + goto error;
> + }
Won't that fail when reading an old-style network status file that has
<network> at the toplevel rather than inside <networkstatus>? Or is
"//xyz" some special XPath notation that means "find this node anywhere
in the hierarchy"?
Yes, //xyz means: "select node matching xyz no matter where they are
from the current node", the current in this case is either
<networkstatus> in case of the new file, or only <network> (and thus
selectable) in case of the old file.
> +
> + /* parse the definition first */
> + ctxt->node = node;
> + if (!(def = virNetworkDefParseXML(ctxt)))
> + goto error;
> +
> + if (!STREQ(name, def->name)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Network config filename '%s'"
> + " does not match network name '%s'"),
> + configFile, def->name);
> + goto error;
> + }
> +
> + if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
> + def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> + def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
> + /* state configs should always have the bridge stored in the file */
> + if (!def->bridge || strstr(def->bridge, "%d")) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("network status XML does not contain valid bridge
"
> + "name"));
> + goto error;
> + }
> + }
Is there a reason this extra validation was needed here? We don't use
def->bridge anywhere else in this function, and I would assume that the
other functions using def after return will be properly validating what
they use.
I'm not really sure if this is needed here. I ported it here from the
persistent config loader function where it validates and fills the
bridge name. Here it should always be filled but I wanted to be sure
that it doesn't break anything when a user would mess with the status files.
If you think it's useless, I'll be happy deleting it :)
> +
> + /* now parse possible status data */
> + node = xmlDocGetRootElement(xml);
> + if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) {
> + /* Newer network status file. Contains useful
> + * info which are not to be found in bare config XML */
> + char *class_id = NULL;
> + char *floor_sum = NULL;
> +
> + ctxt->node = node;
> + if ((class_id = virXPathString("string(./class_id[1]/@bitmap)",
ctxt))) {
> + if (virBitmapParse(class_id, 0, &class_id_map,
> + CLASS_ID_BITMAP_SIZE) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed 'class_id' attribute:
%s"),
> + class_id);
> + VIR_FREE(class_id);
> + goto error;
> + }
> + }
> + VIR_FREE(class_id);
> +
> + floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt);
> + if (floor_sum &&
> + virStrToLong_ull(floor_sum, NULL, 10, &floor_sum_val) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed 'floor_sum' attribute:
%s"),
> + floor_sum);
> + VIR_FREE(floor_sum);
> + }
> + VIR_FREE(floor_sum);
> + }
> +
> + /* create the object */
> + if (!(net = virNetworkAssignDef(nets, def, true)))
> + goto error;
> + /* no goto error below this comment */
You should make that comment less ambiguous, i.e. "do not put any "goto
error" anywhere below this comment"
> +
> + /* assign status data stored in the network object */
> + if (class_id_map) {
> + virBitmapFree(net->class_id);
Rather than calling virBitmapFree here, I think it would be better to
call it after cleanup: (and remove it from error:), just to make it
easier for someone to verify that it will always be properly freed.
You probably misread that. The virBitmapFree call frees the old
definition (note the "net->" )here and assigns a new one that was parsed
from the status config.
> + net->class_id = class_id_map;
> + }
> +
> + if (floor_sum_val > 0)
> + net->floor_sum = floor_sum_val;
> +
> +
> +cleanup:
> + VIR_FREE(configFile);
> + xmlFreeDoc(xml);
> + xmlXPathFreeContext(ctxt);
> + return net;
> +
> +error:
> + virBitmapFree(class_id_map);
... whereas here the status config definition is freed in case it wasn't
used.
> + virNetworkDefFree(def);
> + goto cleanup;
> +}
> +
> virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
> const char *configDir,
> const char *autostartDir,
> @@ -2612,6 +2648,40 @@ error:
> return NULL;
> }
>
> +
> +int
> +virNetworkLoadAllState(virNetworkObjListPtr nets,
> + const char *configDir)
I'll rename this variable too.
> +{
> + DIR *dir;
> + struct dirent *entry;
> +
> + if (!(dir = opendir(configDir))) {
> + if (errno == ENOENT)
> + return 0;
> +
> + virReportSystemError(errno, _("Failed to open dir '%s'"),
configDir);
> + return -1;
> + }
> +
> + while ((entry = readdir(dir))) {
> + virNetworkObjPtr net;
> +
> + if (entry->d_name[0] == '.')
> + continue;
> +
> + if (!virFileStripSuffix(entry->d_name, ".xml"))
> + continue;
> +
> + if ((net = virNetworkLoadState(nets, configDir, entry->d_name)))
> + virNetworkObjUnlock(net);
> + }
> +
> + closedir(dir);
> + return 0;
> +}
> +
> +
> int virNetworkLoadAllConfigs(virNetworkObjListPtr nets,
> const char *configDir,
> const char *autostartDir)
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 1a86e3d..7d4c40b 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -280,9 +280,6 @@ virNetworkDefPtr virNetworkDefParseString(const char *xmlStr);
> virNetworkDefPtr virNetworkDefParseFile(const char *filename);
> virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml,
> xmlNodePtr root);
> -int virNetworkObjUpdateParseFile(const char *filename,
> - virNetworkObjPtr net);
> -
> char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags);
>
> static inline const char *
> @@ -318,10 +315,17 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr
nets,
> const char *autostartDir,
> const char *file);
>
> +virNetworkObjPtr virNetworkLoadState(virNetworkObjListPtr nets,
> + const char *configDir,
> + const char *name);
> +
> int virNetworkLoadAllConfigs(virNetworkObjListPtr nets,
> const char *configDir,
> const char *autostartDir);
>
> +int virNetworkLoadAllState(virNetworkObjListPtr nets,
> + const char *configDir);
> +
> int virNetworkDeleteConfig(const char *configDir,
> const char *autostartDir,
> virNetworkObjPtr net);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 30fdcd7..f778e9c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -454,6 +454,7 @@ virNetworkIpDefNetmask;
> virNetworkIpDefPrefix;
> virNetworkList;
> virNetworkLoadAllConfigs;
> +virNetworkLoadAllState;
> virNetworkObjAssignDef;
> virNetworkObjFree;
> virNetworkObjGetPersistentDef;
> @@ -465,7 +466,6 @@ virNetworkObjSetDefTransient;
> virNetworkObjUnlock;
> virNetworkObjUnsetDefTransient;
> virNetworkObjUpdate;
> -virNetworkObjUpdateParseFile;
> virNetworkRemoveInactive;
> virNetworkSaveConfig;
> virNetworkSaveStatus;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index bca6bd9..56af108 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -181,6 +181,7 @@ networkRemoveInactive(struct network_driver *driver,
> char *radvdconfigfile = NULL;
> char *configfile = NULL;
> char *radvdpidbase = NULL;
> + char *statusfile = NULL;
> dnsmasqContext *dctx = NULL;
> virNetworkDefPtr def = virNetworkObjGetPersistentDef(net);
>
> @@ -202,6 +203,9 @@ networkRemoveInactive(struct network_driver *driver,
> if (!(configfile = networkDnsmasqConfigFileName(def->name)))
> goto no_memory;
>
> + if (!(statusfile = virNetworkConfigFile(NETWORK_STATE_DIR, def->name)))
> + goto no_memory;
> +
> /* dnsmasq */
> dnsmasqDelete(dctx);
> unlink(leasefile);
> @@ -211,6 +215,9 @@ networkRemoveInactive(struct network_driver *driver,
> unlink(radvdconfigfile);
> virPidFileDelete(NETWORK_PID_DIR, radvdpidbase);
>
> + /* remove status file */
> + unlink(statusfile);
> +
> /* remove the network definition */
> virNetworkRemoveInactive(&driver->networks, net);
>
> @@ -221,6 +228,7 @@ cleanup:
> VIR_FREE(configfile);
> VIR_FREE(radvdconfigfile);
> VIR_FREE(radvdpidbase);
> + VIR_FREE(statusfile);
> dnsmasqContextFree(dctx);
> return ret;
>
> @@ -254,33 +262,15 @@ networkBridgeDummyNicName(const char *brname)
> }
>
> static void
> -networkFindActiveConfigs(struct network_driver *driver) {
> +networkFindActiveConfigs(struct network_driver *driver)
> +{
> unsigned int i;
>
> for (i = 0 ; i < driver->networks.count ; i++) {
> virNetworkObjPtr obj = driver->networks.objs[i];
> - char *config;
>
> virNetworkObjLock(obj);
>
> - if ((config = virNetworkConfigFile(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 */
> - if (virNetworkObjUpdateParseFile(config, obj) < 0)
> - VIR_WARN("Unable to update config of '%s' network",
> - obj->def->name);
> - VIR_FREE(config);
> -
> /* If bridge exists, then mark it active */
> if (obj->def->bridge &&
> virNetDevExists(obj->def->bridge) == 1) {
This simple check for the bridge device's existence isn't adequate for
network types that have no bridge, or for network types that have a
bridge that isn't managed by libvirt (i.e. forward
mode='bridge|hostdev'). In those cases libvirt makes no external changes
to the system that could be used to detect if the network is active or
not - the only useful indicator is the presence of the state file. (In
general I think that should be true for networks where libvirt
creates/manages the bridge too; if the state file exists but the bridge
doesn't, that just means that the network needs to be reconstructed, not
that it should be marked inactive).
Well, as that piece of code was pre-existing I didn't dare to touch it.
If we are going to remove this kind of check, we need to make sure that
transient configs (status files) are deleted at reboot of the machine so
that we don't re-create networks on host reboot. Or do we want to do that?.
Also, without this check, the removal code of inactive transient
networks isn't really needed.
> @@ -307,6 +297,21 @@ networkFindActiveConfigs(struct network_driver *driver) {
> cleanup:
> virNetworkObjUnlock(obj);
> }
> +
> +redo:
> + /* remove inactive transient domains */
> + for (i = 0 ; i < driver->networks.count ; i++) {
> + virNetworkObjPtr obj = driver->networks.objs[i];
> +
> + virNetworkObjLock(obj);
> +
> + if (!obj->persistent && !obj->active) {
> + networkRemoveInactive(driver, obj);
I think you have a deadlock here, don't you? networkRemoveInactive()
calls virNetworkRemoveInactive(), which will scan through this same
array, locking each network object before seeing if it's the one we want
to delete. But you've already locked it.
Fortunately, this doesn't deadlock. virNetworkRemoveInactive unlocks the
passed network object before entering the list and scanning through it.
Since we already hold the network driver lock, and we're operating at a
single-threaded place for the network driver anyway, I think the
virNetworkObjLock() here is unnecessary.
Yes, at this point it is not necessary for concurrency but it's
necessary for virNetworkRemoveInactive anyways.
> + goto redo;
It seems a bit drastic to redo all the way from the beginning of the
list. Since this function already assumes enough knowledge about the
list of network objects to know that it's an array, and it knows which
position in the array contains the object we're deleting, we might as
well go all the way and let this code "know" that when an object is
removed, the remainder of the list is moved up by one; then all we need
to do is go to the top of the loop without doing i++.
So, you could just write this as a while loop instead of a for loop, put
the "i++;" at the bottom of the loop, and do a "continue;" instead
of
"goto redo;"
Yeah, I can do that. I wanted to make it implementation agnostic, but as
the array removal implementation is known, I can abuse that knowledge
and save re-iterating (although that shouldn't really be a problem).
> + }
> +
> + virNetworkObjUnlock(obj);
> + }
> }
>
>
> @@ -416,6 +421,10 @@ networkStartup(bool privileged,
> /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */
> driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
>
> + if (virNetworkLoadAllState(&driverState->networks,
> + NETWORK_STATE_DIR) < 0)
> + goto error;
> +
> if (virNetworkLoadAllConfigs(&driverState->networks,
> driverState->networkConfigDir,
> driverState->networkAutostartDir) < 0)
> @@ -480,6 +489,8 @@ networkReload(void) {
> return 0;
>
> networkDriverLock(driverState);
> + virNetworkLoadAllState(&driverState->networks,
> + NETWORK_STATE_DIR);
> virNetworkLoadAllConfigs(&driverState->networks,
> driverState->networkConfigDir,
> driverState->networkAutostartDir);
Otherwise looks good, and will be a nice thing to get fixed.
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Peter
12:18 p.m.
New subject: [libvirt] [PATCH 2/2] network: bridge_driver: don't lose transient networks on daemon restart
On 04/17/2013 04:40 AM, Peter Krempa wrote:
Until now tranisent networks weren't really useful as libvirtd
wasn't
able to remember them across restarts. This patch adds support for
loading status files of transient networks (that already were generated)
so that the status isn't lost.
BTW, can you reference this BZ in your commit log message:
https://bugzilla.redhat.com/show_bug.cgi?id=819416
This patch should resolve that bug.
3:56 a.m.
New subject: [libvirt] [PATCH 2/2] network: bridge_driver: don't lose transient networks on daemon restart
On 04/17/13 10:40, Peter Krempa wrote:
Until now tranisent networks weren't really useful as libvirtd
wasn't
able to remember them across restarts. This patch adds support for
loading status files of transient networks (that already were generated)
so that the status isn't lost.
This patch chops up virNetworkObjUpdateParseFile and turns it into
virNetworkLoadState and a few friends that will help us to load status
XMLs and refactors the functions that are loading the configs to use
them.
---
OOPS :/ sorry for reposting this patch. I didn't clean these patches
from the repo before using git-send email again.
Peter
10:40 a.m.
New subject: [libvirt] [PATCH 2/2] network: bridge_driver: don't lose transient networks on daemon restart
On Wed, Apr 17, 2013 at 10:40:45AM +0200, Peter Krempa wrote:
Until now tranisent networks weren't really useful as libvirtd
wasn't
able to remember them across restarts. This patch adds support for
loading status files of transient networks (that already were generated)
so that the status isn't lost.
This patch chops up virNetworkObjUpdateParseFile and turns it into
virNetworkLoadState and a few friends that will help us to load status
XMLs and refactors the functions that are loading the configs to use
them.
---
src/conf/network_conf.c | 220 +++++++++++++++++++++++++++++---------------
src/conf/network_conf.h | 10 +-
src/libvirt_private.syms | 2 +-
src/network/bridge_driver.c | 51 ++++++----
4 files changed, 184 insertions(+), 99 deletions(-)
diff --git a/src/network/bridge_driver.c
b/src/network/bridge_driver.c
index bca6bd9..56af108 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -416,6 +421,10 @@ networkStartup(bool privileged,
/* if this fails now, it will be retried later with dnsmasqCapsRefresh() */
driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
+ if (virNetworkLoadAllState(&driverState->networks,
+ NETWORK_STATE_DIR) < 0)
+ goto error;
+
if (virNetworkLoadAllConfigs(&driverState->networks,
driverState->networkConfigDir,
driverState->networkAutostartDir) < 0)
@@ -480,6 +489,8 @@ networkReload(void) {
return 0;
networkDriverLock(driverState);
+ virNetworkLoadAllState(&driverState->networks,
+ NETWORK_STATE_DIR);
virNetworkLoadAllConfigs(&driverState->networks,
driverState->networkConfigDir,
driverState->networkAutostartDir);
This has broken launching of libvirtd as non-root, since you cannot
access NETWORK_STATE_DIR - it should be looking under $HOME
$ ./daemon/libvirtd
2013-05-02 15:37:09.753+0000: 14422: info : libvirt version: 1.0.5
2013-05-02 15:37:09.753+0000: 14422: error : virNetworkLoadAllState:2671 : Failed to open
dir '/var/lib/libvirt/network': Permission denied
2013-05-02 15:37:09.753+0000: 14422: error : virStateInitialize:837 : Initialization of
Network state driver failed
2013-05-02 15:37:09.753+0000: 14422: error : daemonRunStateInit:879 : Driver state
initialization failed
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4225
days inactive
4240
days old
9 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Peter Krempa