Re: [libvirt] [PATCH 2/4] network: make virNetworkObjUpdate error detection/recovery better

On 09/21/2012 01:46 PM, Laine Stump 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 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.

ACK.