On 09/21/2012 04:13 PM, Eric Blake wrote:
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. Not necessarily the fastest approach, but also not the first time we've used round-tripping (and reminds me that I still want an API someday
On 09/21/2012 01:46 PM, Laine Stump wrote: that the user can call to canonicalize XML via round-tripping through the parser and formatter).
--- src/conf/network_conf.c | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 34487dd..17b9643 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; Is it worth hoisting this declaration,
if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + virNetworkDefPtr checkdef;
since you use it twice? But no semantic change, so no problem if you don't.
I did it that way on purpose, just to make it clear that it would never need cleaning up at the end of the function.
ACK.
Okay, thanks. I'm pushing this then.