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

[Laine Stump]

On 09/21/2012 04:13 PM, Eric Blake wrote:
> 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.

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.