From patchwork Sun Apr 24 23:22:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cole Robinson X-Patchwork-Id: 66546 Delivered-To: patch@linaro.org Received: by 10.140.93.198 with SMTP id d64csp758308qge; Sun, 24 Apr 2016 16:25:50 -0700 (PDT) X-Received: by 10.28.172.132 with SMTP id v126mr8962313wme.28.1461540350107; Sun, 24 Apr 2016 16:25:50 -0700 (PDT) Return-Path: Received: from mx6-phx2.redhat.com (mx6-phx2.redhat.com. [209.132.183.39]) by mx.google.com with ESMTPS id kb10si21266201wjb.118.2016.04.24.16.25.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 24 Apr 2016 16:25:50 -0700 (PDT) Received-SPF: pass (google.com: domain of libvir-list-bounces@redhat.com designates 209.132.183.39 as permitted sender) client-ip=209.132.183.39; Authentication-Results: mx.google.com; spf=pass (google.com: domain of libvir-list-bounces@redhat.com designates 209.132.183.39 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx6-phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u3ONMx3d041170; Sun, 24 Apr 2016 19:23:01 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u3ONMwgm015465 for ; Sun, 24 Apr 2016 19:22:58 -0400 Received: from colepc.redhat.com (ovpn-113-101.phx2.redhat.com [10.3.113.101]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u3ONMu3U006147; Sun, 24 Apr 2016 19:22:57 -0400 From: Cole Robinson To: libvirt-list@redhat.com Date: Sun, 24 Apr 2016 19:22:50 -0400 Message-Id: In-Reply-To: References: In-Reply-To: References: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-loop: libvir-list@redhat.com Subject: [libvirt] [PATCH 2/4] nwfilter: Fix potential locking problems on ObjLoad failure X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com 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(-) -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list 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;