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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list