We will segfault of a daemon reload picks up a new network config that needs to be autostarted. We shouldn't be passing NULL for network_driver here. This seems like it was missed in the larger rework in commit 1009a61e --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 29c5feb..21e43af 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -769,7 +769,7 @@ networkStateReload(void) networkRefreshDaemons(network_driver); virNetworkObjListForEach(network_driver->networks, networkAutostartConfig, - NULL); + network_driver); return 0; } -- 2.7.3

In virNWFilterObjLoad we can still fail after virNWFilterObjAssignDef, but we don't unlock and free the created virNWFilterObjPtr in the cleanup path. The bit we are trying to do after AssignDef is just STRDUP in the configFile path. However caching the configFile in the NWFilterObj is largely redundant and doesn't follow the same pattern we use for domain and network objects. So just remove all the configFile caching which fixes the latent bug as a side effect. --- src/conf/nwfilter_conf.c | 62 ++++++++++++++++++++---------------------- src/conf/nwfilter_conf.h | 5 ++-- src/nwfilter/nwfilter_driver.c | 6 ++-- 3 files changed, 34 insertions(+), 39 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index ced8eb8..d02bbff 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -353,8 +353,6 @@ virNWFilterObjFree(virNWFilterObjPtr obj) virNWFilterDefFree(obj->def); virNWFilterDefFree(obj->newDef); - VIR_FREE(obj->configFile); - virMutexDestroy(&obj->lock); VIR_FREE(obj); @@ -3181,12 +3179,6 @@ virNWFilterObjLoad(virNWFilterObjListPtr nwfilters, return NULL; } - VIR_FREE(nwfilter->configFile); /* for driver reload */ - if (VIR_STRDUP(nwfilter->configFile, path) < 0) { - virNWFilterDefFree(def); - return NULL; - } - return nwfilter; } @@ -3234,60 +3226,66 @@ virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters, int virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, - virNWFilterObjPtr nwfilter, virNWFilterDefPtr def) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *xml; - int ret; - - if (!nwfilter->configFile) { - if (virFileMakePath(driver->configDir) < 0) { - virReportSystemError(errno, - _("cannot create config directory %s"), - driver->configDir); - return -1; - } + int ret = -1; + char *configFile = NULL; - if (!(nwfilter->configFile = virFileBuildPath(driver->configDir, - def->name, ".xml"))) { - return -1; - } + if (virFileMakePath(driver->configDir) < 0) { + virReportSystemError(errno, + _("cannot create config directory %s"), + driver->configDir); + goto error; + } + + if (!(configFile = virFileBuildPath(driver->configDir, + def->name, ".xml"))) { + goto error; } if (!(xml = virNWFilterDefFormat(def))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to generate XML")); - return -1; + goto error; } virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(nwfilter->configFile, + ret = virXMLSaveFile(configFile, virXMLPickShellSafeComment(def->name, uuidstr), "nwfilter-edit", xml); VIR_FREE(xml); + error: + VIR_FREE(configFile); return ret; } int -virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter) +virNWFilterObjDeleteDef(const char *configDir, + virNWFilterObjPtr nwfilter) { - if (!nwfilter->configFile) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no config file for %s"), nwfilter->def->name); - return -1; + int ret = -1; + char *configFile = NULL; + + if (!(configFile = virFileBuildPath(configDir, + nwfilter->def->name, ".xml"))) { + goto error; } - if (unlink(nwfilter->configFile) < 0) { + if (unlink(configFile) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot remove config for %s"), nwfilter->def->name); - return -1; + goto error; } - return 0; + ret = 0; + error: + VIR_FREE(configFile); + return ret; } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 0211861..823cfa4 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -551,7 +551,6 @@ typedef virNWFilterObj *virNWFilterObjPtr; struct _virNWFilterObj { virMutex lock; - char *configFile; int active; int wantRemoved; @@ -612,10 +611,10 @@ virNWFilterObjPtr virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters, int virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, - virNWFilterObjPtr nwfilter, virNWFilterDefPtr def); -int virNWFilterObjDeleteDef(virNWFilterObjPtr nwfilter); +int virNWFilterObjDeleteDef(const char *configDir, + virNWFilterObjPtr nwfilter); virNWFilterObjPtr virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 1a868b6..2828b28 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -554,7 +554,7 @@ nwfilterDefineXML(virConnectPtr conn, if (!(nwfilter = virNWFilterObjAssignDef(&driver->nwfilters, def))) goto cleanup; - if (virNWFilterObjSaveDef(driver, nwfilter, def) < 0) { + if (virNWFilterObjSaveDef(driver, def) < 0) { virNWFilterObjRemove(&driver->nwfilters, nwfilter); def = NULL; goto cleanup; @@ -602,11 +602,9 @@ nwfilterUndefine(virNWFilterPtr obj) goto cleanup; } - if (virNWFilterObjDeleteDef(nwfilter) < 0) + if (virNWFilterObjDeleteDef(driver->configDir, nwfilter) < 0) goto cleanup; - VIR_FREE(nwfilter->configFile); - virNWFilterObjRemove(&driver->nwfilters, nwfilter); nwfilter = NULL; ret = 0; -- 2.7.3

This matches the pattern used for network object APIs, and we want configDir in LoadConfig for upcoming patches --- src/conf/nwfilter_conf.c | 45 ++++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index d02bbff..d8e83f0 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -3156,30 +3156,39 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, static virNWFilterObjPtr -virNWFilterObjLoad(virNWFilterObjListPtr nwfilters, - const char *file, - const char *path) +virNWFilterLoadConfig(virNWFilterObjListPtr nwfilters, + const char *configDir, + const char *name) { - virNWFilterDefPtr def; + virNWFilterDefPtr def = NULL; virNWFilterObjPtr nwfilter; + char *configFile = NULL; - if (!(def = virNWFilterDefParseFile(path))) - return NULL; + if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) + goto error; - if (!virFileMatchesNameSuffix(file, def->name, ".xml")) { + if (!(def = virNWFilterDefParseFile(configFile))) + goto error; + + if (STRNEQ(name, def->name)) { virReportError(VIR_ERR_XML_ERROR, - _("network filter config filename '%s' does not match name '%s'"), - path, def->name); - virNWFilterDefFree(def); - return NULL; + _("network filter config filename '%s' " + "does not match name '%s'"), + configFile, def->name); + goto error; } if (!(nwfilter = virNWFilterObjAssignDef(nwfilters, def))) { - virNWFilterDefFree(def); - return NULL; + goto error; } + VIR_FREE(configFile); return nwfilter; + + error: + VIR_FREE(configFile); + virNWFilterDefFree(def); + return NULL; } @@ -3200,23 +3209,17 @@ virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters, } while ((ret = virDirRead(dir, &entry, configDir)) > 0) { - char *path; virNWFilterObjPtr nwfilter; if (entry->d_name[0] == '.') continue; - if (!virFileHasSuffix(entry->d_name, ".xml")) - continue; - - if (!(path = virFileBuildPath(configDir, entry->d_name, NULL))) + if (!virFileStripSuffix(entry->d_name, ".xml")) continue; - nwfilter = virNWFilterObjLoad(nwfilters, entry->d_name, path); + nwfilter = virNWFilterLoadConfig(nwfilters, configDir, entry->d_name); if (nwfilter) virNWFilterObjUnlock(nwfilter); - - VIR_FREE(path); } closedir(dir); -- 2.7.3

On 04/24/2016 07:22 PM, Cole Robinson wrote: Make sure you run 'syntax-check'... A window I had used for compiles was obscured and I see the syntax-check fails because of the { } and one line goto error. John

libvirt-daemon-config-nwfilter will put a bunch of xml configs into /etc/libvirt/nwfilter. These configs don't hardcode a UUID and depends on libvirt to generate one. However the generated UUID is never saved to disk, unless the user manually calls Define. This makes daemon reload quite noisy with many errors like: error : virNWFilterObjAssignDef:3101 : operation failed: filter 'allow-incoming-ipv4' already exists with uuid 50def3b5-48d6-46a3-b005-cc22df4e5c5c Because a new UUID is generated every time the config is read from disk, so libvirt constantly thinks it's finding a new nwfilter. Detect if we generated a UUID when the config file is loaded; if so, resave the new contents to disk to ensure the UUID is persisteny. This is similar to what was done in commit a47ae7c0 with virtual networks and generated MAC addresses --- src/conf/nwfilter_conf.c | 6 ++++++ src/conf/nwfilter_conf.h | 1 + 2 files changed, 7 insertions(+) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index d8e83f0..f9cb8ea 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2658,6 +2658,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt) } uuid = virXPathString("string(./uuid)", ctxt); + ret->uuid_specified = (uuid != NULL); if (uuid == NULL) { if (virUUIDGenerate(ret->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3178,6 +3179,11 @@ virNWFilterLoadConfig(virNWFilterObjListPtr nwfilters, goto error; } + /* We generated a UUID, make it permanent by saving the config to disk */ + if (!def->uuid_specified && + virNWFilterSaveConfig(configDir, def) < 0) + goto error; + if (!(nwfilter = virNWFilterObjAssignDef(nwfilters, def))) { goto error; } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 823cfa4..ea3cd5c 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -536,6 +536,7 @@ typedef virNWFilterDef *virNWFilterDefPtr; struct _virNWFilterDef { char *name; unsigned char uuid[VIR_UUID_BUFLEN]; + bool uuid_specified; char *chainsuffix; virNWFilterChainPriority chainPriority; -- 2.7.3