network: make virNetworkObjUpdate error detection/recovery better

1) virNetworkObjUpdate should be an all or none operation, but in the
case that we want to update both the live state and persistent config
versions of the network, it was committing the update to the live
state before starting to update the persistent config. If update of
the persistent config failed, we would leave with things in an
inconsistent state - the live state would be updated (even though an
error was returned), but persistent config unchanged.

This patch changed virNetworkObjUpdate to use a separate pointer for
each copy of the virNetworkDef, and not commit either of them in the
virNetworkObj until both live and config parts of the update have
successfully completed.

2) The parsers for various pieces of the virNetworkDef have all sorts
of subtle limitations on them that may not be known by the
Update[section] function, making it possible for one of these
functions to make a modification directly to the object that may not
pass the scrutiny of a subsequent parse. But normally another parse
wouldn't be done on the data until the *next* time the object was
updated (which could leave the network definition in an unusable
state).

Rather than fighting the losing battle of trying to duplicate all the
checks from the parsers into the update functions as well, the more
foolproof solution to this is to simply do an extra
virNetworkDefCopy() operation on the updated networkdef -
virNetworkDefCopy() does a virNetworkFormat() followed by a
virNetworkParseString(), so it will do all the checks we need. If this
fails, then we don't commit the changed def.

1 files changed, 34 insertions, 13 deletions

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 34487dd4c..17b964335 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2840,45 +2840,66 @@ virNetworkObjUpdate(virNetworkObjPtr network,
                     unsigned int flags)  /* virNetworkUpdateFlags */
 {
     int ret = -1;
-    virNetworkDefPtr def = NULL;
+    virNetworkDefPtr livedef = NULL, configdef = NULL;
 
     /* normalize config data, and check for common invalid requests. */
     if (virNetworkConfigChangeSetup(network, flags) < 0)
        goto cleanup;
 
     if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) {
+        virNetworkDefPtr checkdef;
+
         /* work on a copy of the def */
-        if (!(def = virNetworkDefCopy(network->def, 0)))
+        if (!(livedef = virNetworkDefCopy(network->def, 0)))
             goto cleanup;
-        if (virNetworkDefUpdateSection(def, command, section,
+        if (virNetworkDefUpdateSection(livedef, command, section,
                                        parentIndex, xml, flags) < 0) {
             goto cleanup;
         }
-        /* successfully modified copy, now replace original */
-        virNetworkDefFree(network->def);
-        network->def = def;
-        def = NULL;
+        /* run a final format/parse cycle to make sure we didn't
+         * add anything illegal to the def
+         */
+        if (!(checkdef = virNetworkDefCopy(livedef, 0)))
+            goto cleanup;
+        virNetworkDefFree(checkdef);
     }
 
     if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) {
+        virNetworkDefPtr checkdef;
+
         /* work on a copy of the def */
-        if (!(def = virNetworkDefCopy(virNetworkObjGetPersistentDef(network),
-                                      VIR_NETWORK_XML_INACTIVE))) {
+        if (!(configdef = virNetworkDefCopy(virNetworkObjGetPersistentDef(network),
+                                            VIR_NETWORK_XML_INACTIVE))) {
             goto cleanup;
         }
-        if (virNetworkDefUpdateSection(def, command, section,
+        if (virNetworkDefUpdateSection(configdef, command, section,
                                        parentIndex, xml, flags) < 0) {
             goto cleanup;
         }
+        if (!(checkdef = virNetworkDefCopy(configdef,
+                                           VIR_NETWORK_XML_INACTIVE))) {
+            goto cleanup;
+        }
+        virNetworkDefFree(checkdef);
+    }
+
+    if (configdef) {
         /* successfully modified copy, now replace original */
-        if (virNetworkObjReplacePersistentDef(network, def) < 0)
+        if (virNetworkObjReplacePersistentDef(network, configdef) < 0)
            goto cleanup;
-        def = NULL;
+        configdef = NULL;
+    }
+    if (livedef) {
+        /* successfully modified copy, now replace original */
+        virNetworkDefFree(network->def);
+        network->def = livedef;
+        livedef = NULL;
     }
 
     ret = 0;
 cleanup:
-    virNetworkDefFree(def);
+    virNetworkDefFree(livedef);
+    virNetworkDefFree(configdef);
     return ret;
 }