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

[Eric Blake]

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.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120921/97fa7098/attachment-0001.sig>