network: fix virNetworkObjAssignDef and persistence

Experimentation showed that if virNetworkCreateXML() was called for a
network that was already defined, and then the network was
subsequently shutdown, the network would continue to be persistent
after the shutdown (expected/desired), but the original config would
be lost in favor of the transient config sent in with
virNetworkCreateXML() (which would then be the new persistent config)
(obviously unexpected/not desired).

To fix this, virNetworkObjAssignDef() has been changed to

1) properly save/free network->def and network->newDef for all the
various combinations of live/active/persistent, including some
combinations that were previously considered to be an error but didn't
need to be (e.g. setting a "live" config for a network that isn't yet
active but soon will be - that was previously considered an error,
even though in practice it can be very useful).

2) automatically set the persistent flag whenever a new non-live
config is assigned to the network (and clear it when the non-live
config is set to NULL). the libvirt network driver no longer directly
manipulates network->persistent, but instead relies entirely on
virNetworkObjAssignDef() to do the right thing automatically.

After this patch, the following sequence will behave as expected:

virNetworkDefineXML(X)
virNetworkCreateXML(X') (same name but some config different)
virNetworkDestroy(X)

At the end of these calls, the network config will remain as it was
after the initial virNetworkDefine(), whereas previously it would take
on the changes given during virNetworkCreateXML().

Another effect of this tighter coupling between a) setting a !live def
and b) setting/clearing the "persistent" flag, is that future patches
which change the details of network lifecycle management
(e.g. upcoming patches to fix detection of "active" networks when
libvirtd is restarted) will find it much more difficult to break
persistence functionality.

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 56c4a09d32f341832f7e4afef5236262444cfaea..5527aa4a17dbd58dbfafbee5a8ac76336c7d7e1c 100644 (file)
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -302,46 +302,62 @@ void virNetworkObjListFree(virNetworkObjListPtr nets)
 /*
  * virNetworkObjAssignDef:
  * @network: the network object to update
- * @def: the new NetworkDef (will be consumed by this function iff successful)
+ * @def: the new NetworkDef (will be consumed by this function)
  * @live: is this new def the "live" version, or the "persistent" version
  *
- * Replace the appropriate copy of the given network's NetworkDef
+ * Replace the appropriate copy of the given network's def or newDef
  * with def. Use "live" and current state of the network to determine
- * which to replace.
+ * which to replace and what to do with the old defs. When a non-live
+ * def is set, indicate that the network is now persistent.
+ *
+ * NB: a persistent network can be made transient by calling with:
+ * virNetworkObjAssignDef(network, NULL, false) (i.e. set the
+ * persistent def to NULL)
  *
- * Returns 0 on success, -1 on failure.
  */
-int
+void
 virNetworkObjAssignDef(virNetworkObjPtr network,
                        virNetworkDefPtr def,
                        bool live)
 {
-    if (virNetworkObjIsActive(network)) {
-        if (live) {
+    if (live) {
+        /* before setting new live def, save (into newDef) any
+         * existing persistent (!live) def to be restored when the
+         * network is destroyed, unless there is one already saved.
+         */
+        if (network->persistent && !network->newDef)
+            network->newDef = network->def;
+        else
             virNetworkDefFree(network->def);
-            network->def = def;
-        } else if (network->persistent) {
-            /* save current configuration to be restored on network shutdown */
-            virNetworkDefFree(network->newDef);
+        network->def = def;
+    } else { /* !live */
+        virNetworkDefFree(network->newDef);
+        if (virNetworkObjIsActive(network)) {
+            /* save new configuration to be restored on network
+             * shutdown, leaving current live def alone
+             */
             network->newDef = def;
-        } else {
-            virReportError(VIR_ERR_OPERATION_INVALID,
-                           _("cannot save persistent config of transient "
-                             "network '%s'"), network->def->name);
-            return -1;
+        } else { /* !live and !active */
+            if (network->def && !network->persistent) {
+                /* network isn't (yet) marked active or persistent,
+                 * but already has a "live" def set. This means we are
+                 * currently setting the persistent def as a part of
+                 * the process of starting the network, so we need to
+                 * preserve the "not yet live" def in network->def.
+                 */
+                network->newDef = def;
+            } else {
+                /* either there is no live def set, or this network
+                 * was already set as persistent, so the proper thing
+                 * is to overwrite network->def.
+                 */
+                network->newDef = NULL;
+                virNetworkDefFree(network->def);
+                network->def = def;
+            }
         }
-    } else if (!live) {
-        virNetworkDefFree(network->newDef);
-        virNetworkDefFree(network->def);
-        network->newDef = NULL;
-        network->def = def;
-    } else {
-        virReportError(VIR_ERR_OPERATION_INVALID,
-                       _("cannot save live config of inactive "
-                         "network '%s'"), network->def->name);
-        return -1;
+        network->persistent = !!def;
     }
-    return 0;
 }
 
 /*
@@ -365,10 +381,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
     virNetworkObjPtr network;
 
     if ((network = virNetworkFindByName(nets, def->name))) {
-        if (virNetworkObjAssignDef(network, def, live) < 0) {
-            virNetworkObjUnlock(network);
-            return NULL;
-        }
+        virNetworkObjAssignDef(network, def, live);
         return network;
     }
 
@@ -392,8 +405,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
     ignore_value(virBitmapSetBit(network->class_id, 2));
 
     network->def = def;
-
+    network->persistent = !live;
     return network;
+
  error:
     virNetworkObjUnlock(network);
     virNetworkObjFree(network);
@@ -3118,7 +3132,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
         goto error;
 
     net->autostart = autostart;
-    net->persistent = 1;
 
     VIR_FREE(configFile);
     VIR_FREE(autostartLink);

diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 1a864defec93f9756b8ef2ad33b83d401fc0d463..90da16ffd2375696c3de6ec8ea28958bb437a154 100644 (file)
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,
 virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
                                      virNetworkDefPtr def,
                                      bool live);
-int virNetworkObjAssignDef(virNetworkObjPtr network,
-                           virNetworkDefPtr def,
-                           bool live);
+void virNetworkObjAssignDef(virNetworkObjPtr network,
+                            virNetworkDefPtr def,
+                            bool live);
 int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live);
 void virNetworkObjUnsetDefTransient(virNetworkObjPtr network);
 virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network);

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 201b22f1a1c9045101cf67c6285861402c3f550a..652bccca57ef70c6d4674063cd16b6a835bc26a2 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml)
     if (networkValidate(driver, def, true) < 0)
        goto cleanup;
 
-    /* NB: "live" is false because this transient network hasn't yet
-     * been started
+    /* NB: even though this transient network hasn't yet been started,
+     * we assign the def with live = true in anticipation that it will
+     * be started momentarily.
      */
-    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
+    if (!(network = virNetworkAssignDef(&driver->networks, def, true)))
         goto cleanup;
     def = NULL;
 
@@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
     if (networkValidate(driver, def, false) < 0)
        goto cleanup;
 
-    if ((network = virNetworkFindByName(&driver->networks, def->name))) {
-        network->persistent = 1;
-        if (virNetworkObjAssignDef(network, def, false) < 0)
-            goto cleanup;
-    } else {
-        if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
-            goto cleanup;
-    }
-
-    /* define makes the network persistent - always */
-    network->persistent = 1;
+    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
+       goto cleanup;
 
-    /* def was asigned */
+    /* def was assigned to network object */
     freeDef = false;
 
     if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) {
@@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
             network = NULL;
             goto cleanup;
         }
-        network->persistent = 0;
-        virNetworkDefFree(network->newDef);
-        network->newDef = NULL;
+        /* if network was active already, just undo new persistent
+         * definition by making it transient.
+         * XXX - this isn't necessarily the correct thing to do.
+         */
+        virNetworkObjAssignDef(network, NULL, false);
         goto cleanup;
     }
 
@@ -2788,16 +2782,12 @@ networkUndefine(virNetworkPtr net)
     if (virNetworkObjIsActive(network))
         active = true;
 
+    /* remove autostart link */
     if (virNetworkDeleteConfig(driver->networkConfigDir,
                                driver->networkAutostartDir,
                                network) < 0)
         goto cleanup;
-
-    /* make the network transient */
-    network->persistent = 0;
     network->autostart = 0;
-    virNetworkDefFree(network->newDef);
-    network->newDef = NULL;
 
     event = virNetworkEventLifecycleNew(network->def->name,
                                         network->def->uuid,
@@ -2811,6 +2801,12 @@ networkUndefine(virNetworkPtr net)
             goto cleanup;
         }
         network = NULL;
+    } else {
+
+        /* if the network still exists, it was active, and we need to make
+         * it transient (by deleting the persistent def)
+         */
+        virNetworkObjAssignDef(network, NULL, false);
     }
 
     ret = 0;

diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
index 191dbe16e675d281f14842a1224ae2e670de469b..a45acdc0805ee26884d1644722de6dd9221c0366 100644 (file)
--- a/src/parallels/parallels_network.c
+++ b/src/parallels/parallels_network.c
@@ -231,7 +231,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
         goto cleanup;
     }
     net->active = 1;
-    net->persistent = 1;
     net->autostart = 1;
     virNetworkObjUnlock(net);
     return net;
@@ -267,7 +266,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
         goto cleanup;
     }
     net->active = 1;
-    net->persistent = 1;
     net->autostart = 1;
     virNetworkObjUnlock(net);
 

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 64f3daaeb15fedeef9261d92b2530d4fa0f39148..37756e7ebcff73b7d34fb42de87726ba8acc9b0e 100644 (file)
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -784,7 +784,6 @@ testOpenDefault(virConnectPtr conn)
         goto error;
     }
     netobj->active = 1;
-    netobj->persistent = 1;
     virNetworkObjUnlock(netobj);
 
     if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML)))
@@ -1155,7 +1154,6 @@ testParseNetworks(testConnPtr privconn,
             goto error;
         }
 
-        obj->persistent = 1;
         obj->active = 1;
         virNetworkObjUnlock(obj);
     }
@@ -3711,7 +3709,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml)
     if ((def = virNetworkDefParseString(xml)) == NULL)
         goto cleanup;
 
-    if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
+    if (!(net = virNetworkAssignDef(&privconn->networks, def, true)))
         goto cleanup;
     def = NULL;
     net->active = 1;
@@ -3748,7 +3746,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml)
     if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
         goto cleanup;
     def = NULL;
-    net->persistent = 1;
 
     event = virNetworkEventLifecycleNew(net->def->name, net->def->uuid,
                                         VIR_NETWORK_EVENT_DEFINED,