On 04/24/2016 07:22 PM, Cole Robinson wrote:
> 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(-)
>
Fine with the concept - seems as though there's still some streamlining
that can be done - possibly as a followup sometime. Seeing as
virNWFilterConfigFile essentially does the same thing as virFileBuildPath.
Trying to compare with network (and domain) would then just be easier.
Those are the models I used for secrets.
ACK to what's here (extra credit saved for the future).
John
> 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;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list