On 04/23/2014 09:49 AM, Laine Stump wrote:
> Experimentation showed that if virNetworkCreateXML() was called for a
> network that was already defined, and then the network was
> subsequently shutdown, the network would continue to be persistent
> after the shutdown (expected/desired), but the original config would
> be lost in favor of the transient config sent in with
> virNetworkCreateXML() (which would then be the new persistent config)
> (obviously unexpected/not desired).
>
> To fix this, virNetworkObjAssignDef() has been changed to
>
> 1) properly save/free network->def and network->newDef for all the
> various combinations of live/active/persistent, including some
> combinations that were previously considered to be an error but didn't
> need to be (e.g. setting a "live" config for a network that isn't yet
> active but soon will be - that was previously considered an error,
> even though in practice it can be very useful).
>
> 2) automatically set the persistent flag whenever a new non-live
> config is assigned to the network (and clear it when the non-live
> config is set to NULL). the libvirt network driver no longer directly
> manipulates network->persistent, but instead relies entirely on
> virNetworkObjAssignDef() to do the right thing automatically.
>
> After this patch, the following sequence will behave as expected:
>
> virNetworkDefineXML(X)
> virNetworkCreateXML(X') (same name but some config different)
> virNetworkDestroy(X)
>
> At the end of these calls, the network config will remain as it was
> after the initial virNetworkDefine(), whereas previously it would take
> on the changes given during virNetworkCreateXML().
>
> Another effect of this tighter coupling between a) setting a !live def
> and b) setting/clearing the "persistent" flag, is that future patches
> which change the details of network lifecycle management
> (e.g. upcoming patches to fix detection of "active" networks when
> libvirtd is restarted) will find it much more difficult to break
> persistence functionality.
> ---
> src/conf/network_conf.c | 78 +++++++++++++++++++++++----------------
> src/conf/network_conf.h | 6 +--
> src/network/bridge_driver.c | 34 +++++++----------
> src/parallels/parallels_network.c | 4 +-
> src/test/test_driver.c | 5 +--
> 5 files changed, 64 insertions(+), 63 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 56c4a09..66945ce 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets)
> /*
> * virNetworkObjAssignDef:
> * @network: the network object to update
> - * @def: the new NetworkDef (will be consumed by this function iff successful)
> + * @def: the new NetworkDef (will be consumed by this function)
> * @live: is this new def the "live" version, or the "persistent" version
> *
> - * Replace the appropriate copy of the given network's NetworkDef
> + * Replace the appropriate copy of the given network's def or newDef
> * with def. Use "live" and current state of the network to determine
> - * which to replace.
> + * which to replace and what to do with the old defs. When a non-live
> + * def is set, indicate that the network is now persistent.
> + *
> + * NB: a persistent network can be made transient by calling with:
> + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the
> + * persistent def to NULL)
> *
> * Returns 0 on success, -1 on failure.
> */
> -int
> +void
Well this doesn't jive with the comment above about what it returns...
> virNetworkObjAssignDef(virNetworkObjPtr network,
> virNetworkDefPtr def,
> bool live)
> {
> - if (virNetworkObjIsActive(network)) {
> - if (live) {
> + if (live) {
> + /* before setting new live def, save (into newDef) any
> + * existing persistent (!live) def to be restored when the
> + * network is destroyed, unless there is one already saved.
> + */
> + if (network->persistent && !network->newDef)
> + network->newDef = network->def;
> + else
> virNetworkDefFree(network->def);
> - network->def = def;
> - } else if (network->persistent) {
> - /* save current configuration to be restored on network shutdown */
> - virNetworkDefFree(network->newDef);
> + network->def = def;
> + } else { /* !live */
> + virNetworkDefFree(network->newDef);
> + if (virNetworkObjIsActive(network)) {
> + /* save new configuration to be restored on network
> + * shutdown, leaving current live def alone
> + */
> network->newDef = def;
> - } else {
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("cannot save persistent config of transient "
> - "network '%s'"), network->def->name);
> - return -1;
> + } else { /* !live and !active */
> + if (network->def && !network->persistent) {
> + /* network isn't (yet) marked active or persistent,
> + * but already has a "live" def set. This means we are
> + * currently setting the persistent def as a part of
> + * the process of starting the network, so we need to
> + * preserve the "not yet live" def in network->def.
> + */
> + network->newDef = def;
> + } else {
> + /* either there is no live def set, or this network
> + * was already set as persistent, so the proper thing
> + * is to overwrite network->def.
> + */
> + network->newDef = NULL;
If I'm reading correctly - the newDef is virNetworkDefFree()'d above as
we enter the "} else { /* !live */" clause, which is probably where this
statement should be moved.
> + virNetworkDefFree(network->def);
> + network->def = def;
NOTE: I think this is what is causing issues in networkUndefine() -
that'll call this function with '!live' - thus you'll do the free the
'def' and set it to NULL... Back in the caller there's all sorts of
touching of the network->def-> structure
> + }
> }
> - } else if (!live) {
> - virNetworkDefFree(network->newDef);
> - virNetworkDefFree(network->def);
> - network->newDef = NULL;
> - network->def = def;
> - } else {
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("cannot save live config of inactive "
> - "network '%s'"), network->def->name);
> - return -1;
> + network->persistent = !!def;
> }
> - return 0;
> }
>
> /*
> @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
> virNetworkObjPtr network;
>
> if ((network = virNetworkFindByName(nets, def->name))) {
> - if (virNetworkObjAssignDef(network, def, live) < 0) {
> - virNetworkObjUnlock(network);
> - return NULL;
> - }
> + virNetworkObjAssignDef(network, def, live);
> return network;
> }
>
> @@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
> ignore_value(virBitmapSetBit(network->class_id, 2));
>
> network->def = def;
> -
> + network->persistent = !live;
> return network;
> +
> error:
> virNetworkObjUnlock(network);
> virNetworkObjFree(network);
> @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
> goto error;
>
> net->autostart = autostart;
> - net->persistent = 1;
>
> VIR_FREE(configFile);
> VIR_FREE(autostartLink);
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 1a864de..90da16f 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,
> virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
> virNetworkDefPtr def,
> bool live);
> -int virNetworkObjAssignDef(virNetworkObjPtr network,
> - virNetworkDefPtr def,
> - bool live);
> +void virNetworkObjAssignDef(virNetworkObjPtr network,
> + virNetworkDefPtr def,
> + bool live);
> int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live);
> void virNetworkObjUnsetDefTransient(virNetworkObjPtr network);
> virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network);
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index eb276cd..62d1c25 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml)
> if (networkValidate(driver, def, true) < 0)
> goto cleanup;
>
> - /* NB: "live" is false because this transient network hasn't yet
> - * been started
> + /* NB: even though this transient network hasn't yet been started,
> + * we assign the def with live = true in anticipation that it will
> + * be started momentarily.
> */
> - if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
> + if (!(network = virNetworkAssignDef(&driver->networks, def, true)))
> goto cleanup;
> def = NULL;
>
> @@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
> if (networkValidate(driver, def, false) < 0)
> goto cleanup;
>
> - if ((network = virNetworkFindByName(&driver->networks, def->name))) {
> - network->persistent = 1;
> - if (virNetworkObjAssignDef(network, def, false) < 0)
> - goto cleanup;
> - } else {
> - if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
> - goto cleanup;
> - }
> -
> - /* define makes the network persistent - always */
> - network->persistent = 1;
> + if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
> + goto cleanup;
>
> - /* def was asigned */
> + /* def was assigned to network object */
> freeDef = false;
>
> if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) {
> @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
> network = NULL;
> goto cleanup;
> }
> - network->persistent = 0;
> - virNetworkDefFree(network->newDef);
> - network->newDef = NULL;
> + /* if network was active already, just undo new persistent
> + * definition by making it transient.
> + * XXX - this isn't necessarily the correct thing to do.
> + */
Leaving the XXX comment in here leaves me wondering what is the correct
thing to do...
> + virNetworkObjAssignDef(network, NULL, false);
> goto cleanup;
> }
>
> @@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net)
> goto cleanup;
>
> /* make the network transient */
> - network->persistent = 0;
> network->autostart = 0;
> - virNetworkDefFree(network->newDef);
> - network->newDef = NULL;
> + virNetworkObjAssignDef(network, NULL, false);
This seems to be the cause of the crash I noted in private chat. If I
restore just these lines and not make the ObjAssignDef() call, then
things work again. That includes issues I was seeing in virt-test for
patches 3 & 4 as well.
Just figured I'd close the loop on this review at least...
John
>
> event = virNetworkEventLifecycleNew(network->def->name,
> network->def->uuid,
> diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
> index cbb44b9..0ebeb7b 100644
> --- a/src/parallels/parallels_network.c
> +++ b/src/parallels/parallels_network.c
> @@ -2,7 +2,7 @@
> * parallels_network.c: core privconn functions for managing
> * Parallels Cloud Server hosts
> *
> - * Copyright (C) 2013 Red Hat, Inc.
> + * Copyright (C) 2013-2014 Red Hat, Inc.
> * Copyright (C) 2012 Parallels, Inc.
> *
> * This library is free software; you can redistribute it and/or
> @@ -231,7 +231,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
> goto cleanup;
> }
> net->active = 1;
> - net->persistent = 1;
> net->autostart = 1;
> virNetworkObjUnlock(net);
> return net;
> @@ -267,7 +266,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
> goto cleanup;
> }
> net->active = 1;
> - net->persistent = 1;
> net->autostart = 1;
> virNetworkObjUnlock(net);
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 64f3daa..37756e7 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -784,7 +784,6 @@ testOpenDefault(virConnectPtr conn)
> goto error;
> }
> netobj->active = 1;
> - netobj->persistent = 1;
> virNetworkObjUnlock(netobj);
>
> if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML)))
> @@ -1155,7 +1154,6 @@ testParseNetworks(testConnPtr privconn,
> goto error;
> }
>
> - obj->persistent = 1;
> obj->active = 1;
> virNetworkObjUnlock(obj);
> }
> @@ -3711,7 +3709,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml)
> if ((def = virNetworkDefParseString(xml)) == NULL)
> goto cleanup;
>
> - if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
> + if (!(net = virNetworkAssignDef(&privconn->networks, def, true)))
> goto cleanup;
> def = NULL;
> net->active = 1;
> @@ -3748,7 +3746,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml)
> if (!(net = virNetworkAssignDef(&privconn->networks, def, false)))
> goto cleanup;
> def = NULL;
> - net->persistent = 1;
>
> event = virNetworkEventLifecycleNew(net->def->name, net->def->uuid,
> VIR_NETWORK_EVENT_DEFINED,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list