https://bugzilla.redhat.com/show_bug.cgi?id=866364 pointed out a crash due to virNetworkObjAssignDef free'ing network->newDef without NULLing it afterward. A fix for this is in upstream commit b7e9202401ebaa039b8f05acdefda8c24081537a. While the NULLing of newDef was a legitimate fix, newDef should have already been empty (NULL) anyway (as indicated in the comment that was deleted by that commit). The reason that newDef had a non-NULL value (i.e. the root cause) was that networkStartNetwork() had failed after populating network->newDef, but then neglected to free/NULL newDef in the cleanup. (A bit of background here: network->newDef should contain the persistent config of a network when a network is active (and of course only when it is persisten), and NULL at all other times. There is also a network->def which should contain the persistent definition of the network when it is inactive, and the current live state at all other times. The idea is that you can make changes to network->newDef which will take effect the next time the network is restarted, but won't mess with the current state of the network (virDomainObj has a similar pair of virDomainDefs that behave in the same fashion). Personally I think there should be a network->live and network->config, and the location of the persistent config should *always* be in network->config, but that's for a later cleanup). Since I love things to be symmetric, I created a new function called virNetworkObjUnsetDefTransient(), which reverses the effects of virNetworkObjSetDefTransient(). I don't really like the name of the new function, but then I also didn't really like the name of the old one either (it's just named that way to match a similar function in the domain conf code). --- src/conf/network_conf.c | 14 ++++++++++++++ src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 12 ++++-------- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 0f7470d..2f9ad2e 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -352,6 +352,20 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) return network->newDef ? 0 : -1; } +/* virNetworkObjUnsetDefTransient: + * + * This *undoes* what virNetworkObjSetDefTransient did. + */ +void +virNetworkObjUnsetDefTransient(virNetworkObjPtr network) +{ + if (network->def) { + virNetworkDefFree(network->def); + network->def = network->newDef; + network->newDef = NULL; + } +} + /* * virNetworkObjGetPersistentDef: * @network: network object pointer diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 55502fb..4c0c8c1 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -253,6 +253,7 @@ int virNetworkObjAssignDef(virNetworkObjPtr network, const virNetworkDefPtr def, bool live); int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live); +void virNetworkObjUnsetDefTransient(virNetworkObjPtr network); virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network); int virNetworkObjReplacePersistentDef(virNetworkObjPtr network, virNetworkDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1a0c2f6..fd30024 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -872,6 +872,7 @@ virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; virNetworkObjUnlock; +virNetworkObjUnsetDefTransient; virNetworkObjUpdate; virNetworkRemoveInactive; virNetworkSaveConfig; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5578373..fa909a1 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2327,8 +2327,10 @@ networkStartNetwork(struct network_driver *driver, break; } - if (ret < 0) + if (ret < 0) { + virNetworkObjUnsetDefTransient(network); return ret; + } /* Persist the live configuration now that anything autogenerated * is setup. @@ -2388,13 +2390,7 @@ static int networkShutdownNetwork(struct network_driver *driver, } network->active = 0; - - if (network->newDef) { - virNetworkDefFree(network->def); - network->def = network->newDef; - network->newDef = NULL; - } - + virNetworkObjUnsetDefTransient(network); return ret; } -- 1.7.11.7