Re: [libvirt] [PATCHv2 0.5/4] network: fix virNetworkObjAssignDef and persistence

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...