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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list