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

On 04/24/2014 07:09 PM, John Ferlan wrote: > > 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... Yeah, when all the failure paths disappeared I changed the function to void, but forgot to remove that comment. I'll fix that now. >> 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. No, I just did some reduction of common code by moving it above the if statement. You'll notice that network->newDef is set to *something* in every path through those ifs and elses, so at the end of the function, it is either NULL or pointing to a valid object. > >> + 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 Yes, indirectly. But the *real* problem is that we shouldn't be calling this function until after we're finished using anything in network->def. See below for the small diff I'm squashing in to fix this. > > >> + } >> } >> - } 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... The correct thing to do is... well, to exactly rollback to what you had before the failure. However, the current code (both before *and* after this patch) does the (same) wrong thing in this case - if you fail during an attempt to redefine an already persistent+active network, you'll end up with a transient+active network (so it will disappear as soon as the network is stopped). It would be better if such a failure caused us to revert back to the previous persistent def. That would require saving off the existing def before trying to assign the new one or writing to disk. But that is a separate problem and should be in a separate patch anyway. The only reason I touched that area of the code in this patch was that I was replacing direct assignment of network->persistent with calling virNetworkObjAssignDef() (while preserving the existing functionality). I did think it was worth noting the problem with the existing functionality while touching the code, however. So, while I think this needs fixing, it's pre-existing and I'd rather not have these other patches delayed because of it. > >> + 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. Yep. That call needs to be moved down to below the call to networkRemoveInactive. I've done that and all the tests complete without failure (as you've also separately reported on IRC). Here is what I squashed into this patch: diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 309d967..2a54aa3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2899,14 +2899,12 @@ networkUndefine(virNetworkPtr net) if (virNetworkObjIsActive(network)) active = true; + /* remove autostart link */ if (virNetworkDeleteConfig(driver->networkConfigDir, driver->networkAutostartDir, network) < 0) goto cleanup; - - /* make the network transient */ network->autostart = 0; - virNetworkObjAssignDef(network, NULL, false); event = virNetworkEventLifecycleNew(network->def->name, network->def->uuid, @@ -2920,6 +2918,12 @@ networkUndefine(virNetworkPtr net) goto cleanup; } network = NULL; + } else { + + /* if the network still exists, it was active, and we need to make + * it transient (by deleting the persistent def

+ */ + virNetworkObjAssignDef(network, NULL, false); } ret = 0; -- > > Just figured I'd close the loop on this review at least... Do my responses clear up your concerns?