[libvirt] [PATCH 00/15] Multiple cleanups within nwfilterobj and nwfilter drivers
More adjustments in preparation for having virobject code handle the bulk of the object management code. I know it's a lot of patches, but reducing the amount of change overall has the opposite effect of increasing the number patches to show the work flow. Should make the "next" steps easier to review, but a couple of these will be tedious. John Ferlan (15): nwfilter: Use consistent naming for variables nwfilter: Use virNWFilterDefPtr rather than deref virNWFilterObjPtr nwfilter: Remove unused 'active' in virNWFilterObj nwfilter: Convert wantRemoved to bool nwfilter: Make _virNWFilterObjPtr private nwfilter: Introduce virNWFilterObjNew nwfilter: Rename some virNWFilterObj* API's nwfilter: Make _virNWFilterObjList private nwfilter: Make a common UUID lookup function from driver nwfilter: Replace virNWFilterConfigFile with virFileBuildPath nwfilter: Replace virNWFilterSaveDef with virNWFilterSaveConfig nwfilter: Move creation of configDir to driver initialization nwfilter: Move configFile name processing into driver nwfilter: Move save of config until after successful assign nwfilter: Move and rename virNWFilterSaveConfig src/conf/nwfilter_conf.c | 107 +----------- src/conf/nwfilter_conf.h | 16 +- src/conf/virnwfilterobj.c | 308 ++++++++++++++++++++++----------- src/conf/virnwfilterobj.h | 74 ++++---- src/libvirt_private.syms | 21 ++- src/nwfilter/nwfilter_driver.c | 148 ++++++++++------ src/nwfilter/nwfilter_gentech_driver.c | 42 +++-- 7 files changed, 377 insertions(+), 339 deletions(-) -- 2.9.3
When processing a virNWFilterPtr use 'nwfilter' as a variable name. When processing a virNWFilterObjPtr use 'obj' as a variable name. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 92 +++++++++++++++++++++--------------------- src/conf/virnwfilterobj.h | 7 ++-- src/nwfilter/nwfilter_driver.c | 78 +++++++++++++++++------------------ 3 files changed, 89 insertions(+), 88 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 34d843c..cb697f9 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -62,15 +62,15 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) void virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, - virNWFilterObjPtr nwfilter) + virNWFilterObjPtr obj) { size_t i; - virNWFilterObjUnlock(nwfilter); + virNWFilterObjUnlock(obj); for (i = 0; i < nwfilters->count; i++) { virNWFilterObjLock(nwfilters->objs[i]); - if (nwfilters->objs[i] == nwfilter) { + if (nwfilters->objs[i] == obj) { virNWFilterObjUnlock(nwfilters->objs[i]); virNWFilterObjFree(nwfilters->objs[i]); @@ -174,16 +174,16 @@ virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, int -virNWFilterObjTestUnassignDef(virNWFilterObjPtr nwfilter) +virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj) { int rc = 0; - nwfilter->wantRemoved = 1; + obj->wantRemoved = 1; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild()) rc = -1; - nwfilter->wantRemoved = 0; + obj->wantRemoved = 0; return rc; } @@ -225,29 +225,29 @@ virNWFilterObjPtr virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def) { - virNWFilterObjPtr nwfilter; + virNWFilterObjPtr obj; - nwfilter = virNWFilterObjFindByUUID(nwfilters, def->uuid); + obj = virNWFilterObjFindByUUID(nwfilters, def->uuid); - if (nwfilter) { - if (STRNEQ(def->name, nwfilter->def->name)) { + if (obj) { + if (STRNEQ(def->name, obj->def->name)) { virReportError(VIR_ERR_OPERATION_FAILED, _("filter with same UUID but different name " "('%s') already exists"), - nwfilter->def->name); - virNWFilterObjUnlock(nwfilter); + obj->def->name); + virNWFilterObjUnlock(obj); return NULL; } - virNWFilterObjUnlock(nwfilter); + virNWFilterObjUnlock(obj); } else { - nwfilter = virNWFilterObjFindByName(nwfilters, def->name); - if (nwfilter) { + obj = virNWFilterObjFindByName(nwfilters, def->name); + if (obj) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(nwfilter->def->uuid, uuidstr); + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virNWFilterObjUnlock(nwfilter); + virNWFilterObjUnlock(obj); return NULL; } } @@ -259,49 +259,49 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, } - if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) { + if ((obj = virNWFilterObjFindByName(nwfilters, def->name))) { - if (virNWFilterDefEqual(def, nwfilter->def, false)) { - virNWFilterDefFree(nwfilter->def); - nwfilter->def = def; - return nwfilter; + if (virNWFilterDefEqual(def, obj->def, false)) { + virNWFilterDefFree(obj->def); + obj->def = def; + return obj; } - nwfilter->newDef = def; + obj->newDef = def; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild()) { - nwfilter->newDef = NULL; - virNWFilterObjUnlock(nwfilter); + obj->newDef = NULL; + virNWFilterObjUnlock(obj); return NULL; } - virNWFilterDefFree(nwfilter->def); - nwfilter->def = def; - nwfilter->newDef = NULL; - return nwfilter; + virNWFilterDefFree(obj->def); + obj->def = def; + obj->newDef = NULL; + return obj; } - if (VIR_ALLOC(nwfilter) < 0) + if (VIR_ALLOC(obj) < 0) return NULL; - if (virMutexInitRecursive(&nwfilter->lock) < 0) { + if (virMutexInitRecursive(&obj->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - VIR_FREE(nwfilter); + VIR_FREE(obj); return NULL; } - virNWFilterObjLock(nwfilter); - nwfilter->active = 0; + virNWFilterObjLock(obj); + obj->active = 0; if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, - nwfilters->count, nwfilter) < 0) { - virNWFilterObjUnlock(nwfilter); - virNWFilterObjFree(nwfilter); + nwfilters->count, obj) < 0) { + virNWFilterObjUnlock(obj); + virNWFilterObjFree(obj); return NULL; } - nwfilter->def = def; + obj->def = def; - return nwfilter; + return obj; } @@ -414,7 +414,7 @@ virNWFilterObjLoadConfig(virNWFilterObjListPtr nwfilters, const char *name) { virNWFilterDefPtr def = NULL; - virNWFilterObjPtr nwfilter; + virNWFilterObjPtr obj; char *configFile = NULL; if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) @@ -436,11 +436,11 @@ virNWFilterObjLoadConfig(virNWFilterObjListPtr nwfilters, virNWFilterSaveConfig(configDir, def) < 0) goto error; - if (!(nwfilter = virNWFilterObjAssignDef(nwfilters, def))) + if (!(obj = virNWFilterObjAssignDef(nwfilters, def))) goto error; VIR_FREE(configFile); - return nwfilter; + return obj; error: VIR_FREE(configFile); @@ -462,14 +462,14 @@ virNWFilterObjLoadAllConfigs(virNWFilterObjListPtr nwfilters, return rc; while ((ret = virDirRead(dir, &entry, configDir)) > 0) { - virNWFilterObjPtr nwfilter; + virNWFilterObjPtr obj; if (!virFileStripSuffix(entry->d_name, ".xml")) continue; - nwfilter = virNWFilterObjLoadConfig(nwfilters, configDir, entry->d_name); - if (nwfilter) - virNWFilterObjUnlock(nwfilter); + obj = virNWFilterObjLoadConfig(nwfilters, configDir, entry->d_name); + if (obj) + virNWFilterObjUnlock(obj); } VIR_DIR_CLOSE(dir); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 49b1170..2adffd9 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -62,9 +62,10 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters); void virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, - virNWFilterObjPtr nwfilter); + virNWFilterObjPtr obj); -void virNWFilterObjFree(virNWFilterObjPtr obj); +void +virNWFilterObjFree(virNWFilterObjPtr obj); virNWFilterObjPtr virNWFilterObjFindByUUID(virNWFilterObjListPtr nwfilters, @@ -79,7 +80,7 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def); int -virNWFilterObjTestUnassignDef(virNWFilterObjPtr nwfilter); +virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj); typedef bool (*virNWFilterObjListFilter)(virConnectPtr conn, diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index f6c419c..72b9b8e 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -357,27 +357,27 @@ static virNWFilterPtr nwfilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - virNWFilterObjPtr nwfilter; + virNWFilterObjPtr obj; virNWFilterPtr ret = NULL; nwfilterDriverLock(); - nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, uuid); + obj = virNWFilterObjFindByUUID(&driver->nwfilters, uuid); nwfilterDriverUnlock(); - if (!nwfilter) { + if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, "%s", _("no nwfilter with matching uuid")); goto cleanup; } - if (virNWFilterLookupByUUIDEnsureACL(conn, nwfilter->def) < 0) + if (virNWFilterLookupByUUIDEnsureACL(conn, obj->def) < 0) goto cleanup; - ret = virGetNWFilter(conn, nwfilter->def->name, nwfilter->def->uuid); + ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid); cleanup: - if (nwfilter) - virNWFilterObjUnlock(nwfilter); + if (obj) + virNWFilterObjUnlock(obj); return ret; } @@ -386,27 +386,27 @@ static virNWFilterPtr nwfilterLookupByName(virConnectPtr conn, const char *name) { - virNWFilterObjPtr nwfilter; + virNWFilterObjPtr obj; virNWFilterPtr ret = NULL; nwfilterDriverLock(); - nwfilter = virNWFilterObjFindByName(&driver->nwfilters, name); + obj = virNWFilterObjFindByName(&driver->nwfilters, name); nwfilterDriverUnlock(); - if (!nwfilter) { + if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, _("no nwfilter with matching name '%s'"), name); goto cleanup; } - if (virNWFilterLookupByNameEnsureACL(conn, nwfilter->def) < 0) + if (virNWFilterLookupByNameEnsureACL(conn, obj->def) < 0) goto cleanup; - ret = virGetNWFilter(conn, nwfilter->def->name, nwfilter->def->uuid); + ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid); cleanup: - if (nwfilter) - virNWFilterObjUnlock(nwfilter); + if (obj) + virNWFilterObjUnlock(obj); return ret; } @@ -466,7 +466,7 @@ nwfilterDefineXML(virConnectPtr conn, const char *xml) { virNWFilterDefPtr def; - virNWFilterObjPtr nwfilter = NULL; + virNWFilterObjPtr obj = NULL; virNWFilterPtr ret = NULL; if (!driver->privileged) { @@ -485,22 +485,22 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterDefineXMLEnsureACL(conn, def) < 0) goto cleanup; - if (!(nwfilter = virNWFilterObjAssignDef(&driver->nwfilters, def))) + if (!(obj = virNWFilterObjAssignDef(&driver->nwfilters, def))) goto cleanup; if (virNWFilterSaveDef(driver->configDir, def) < 0) { - virNWFilterObjRemove(&driver->nwfilters, nwfilter); + virNWFilterObjRemove(&driver->nwfilters, obj); def = NULL; goto cleanup; } def = NULL; - ret = virGetNWFilter(conn, nwfilter->def->name, nwfilter->def->uuid); + ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid); cleanup: virNWFilterDefFree(def); - if (nwfilter) - virNWFilterObjUnlock(nwfilter); + if (obj) + virNWFilterObjUnlock(obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -510,42 +510,42 @@ nwfilterDefineXML(virConnectPtr conn, static int -nwfilterUndefine(virNWFilterPtr obj) +nwfilterUndefine(virNWFilterPtr nwfilter) { - virNWFilterObjPtr nwfilter; + virNWFilterObjPtr obj; int ret = -1; nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); - if (!nwfilter) { + obj = virNWFilterObjFindByUUID(&driver->nwfilters, nwfilter->uuid); + if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, "%s", _("no nwfilter with matching uuid")); goto cleanup; } - if (virNWFilterUndefineEnsureACL(obj->conn, nwfilter->def) < 0) + if (virNWFilterUndefineEnsureACL(nwfilter->conn, obj->def) < 0) goto cleanup; - if (virNWFilterObjTestUnassignDef(nwfilter) < 0) { + if (virNWFilterObjTestUnassignDef(obj) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("nwfilter is in use")); goto cleanup; } - if (virNWFilterDeleteDef(driver->configDir, nwfilter->def) < 0) + if (virNWFilterDeleteDef(driver->configDir, obj->def) < 0) goto cleanup; - virNWFilterObjRemove(&driver->nwfilters, nwfilter); - nwfilter = NULL; + virNWFilterObjRemove(&driver->nwfilters, obj); + obj = NULL; ret = 0; cleanup: - if (nwfilter) - virNWFilterObjUnlock(nwfilter); + if (obj) + virNWFilterObjUnlock(obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -555,32 +555,32 @@ nwfilterUndefine(virNWFilterPtr obj) static char * -nwfilterGetXMLDesc(virNWFilterPtr obj, +nwfilterGetXMLDesc(virNWFilterPtr nwfilter, unsigned int flags) { - virNWFilterObjPtr nwfilter; + virNWFilterObjPtr obj; char *ret = NULL; virCheckFlags(0, NULL); nwfilterDriverLock(); - nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid); + obj = virNWFilterObjFindByUUID(&driver->nwfilters, nwfilter->uuid); nwfilterDriverUnlock(); - if (!nwfilter) { + if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, "%s", _("no nwfilter with matching uuid")); goto cleanup; } - if (virNWFilterGetXMLDescEnsureACL(obj->conn, nwfilter->def) < 0) + if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, obj->def) < 0) goto cleanup; - ret = virNWFilterDefFormat(nwfilter->def); + ret = virNWFilterDefFormat(obj->def); cleanup: - if (nwfilter) - virNWFilterObjUnlock(nwfilter); + if (obj) + virNWFilterObjUnlock(obj); return ret; } -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:30PM -0400, John Ferlan wrote:
When processing a virNWFilterPtr use 'nwfilter' as a variable name.
When processing a virNWFilterObjPtr use 'obj' as a variable name.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 92 +++++++++++++++++++++--------------------- src/conf/virnwfilterobj.h | 7 ++-- src/nwfilter/nwfilter_driver.c | 78 +++++++++++++++++------------------ 3 files changed, 89 insertions(+), 88 deletions(-)
I'll just loudly object that this is not necessary but on the other hand it makes following the code through different functions easier. Weak ACK, if there are on objections from others. Pavel
Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X create local virNWFilterObjPtr and virNWFilterDefPtr variables. Future adjustments will be privatizing the object more, so this just prepares the code for that reality. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++--------------- src/nwfilter/nwfilter_driver.c | 33 ++++++++++------- 2 files changed, 72 insertions(+), 41 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index cb697f9..3c6bdbb 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj"); void virNWFilterObjFree(virNWFilterObjPtr obj) { + virNWFilterDefPtr def; + virNWFilterDefPtr newDef; + if (!obj) return; + def = obj->def; + newDef = obj->newDef; - virNWFilterDefFree(obj->def); - virNWFilterDefFree(obj->newDef); + virNWFilterDefFree(def); + virNWFilterDefFree(newDef); virMutexDestroy(&obj->lock); @@ -87,12 +92,16 @@ virNWFilterObjFindByUUID(virNWFilterObjListPtr nwfilters, const unsigned char *uuid) { size_t i; + virNWFilterObjPtr obj; + virNWFilterDefPtr def; for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjLock(nwfilters->objs[i]); - if (!memcmp(nwfilters->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return nwfilters->objs[i]; - virNWFilterObjUnlock(nwfilters->objs[i]); + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + def = obj->def; + if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) + return obj; + virNWFilterObjUnlock(obj); } return NULL; @@ -104,12 +113,16 @@ virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters, const char *name) { size_t i; + virNWFilterObjPtr obj; + virNWFilterDefPtr def; for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjLock(nwfilters->objs[i]); - if (STREQ_NULLABLE(nwfilters->objs[i]->def->name, name)) - return nwfilters->objs[i]; - virNWFilterObjUnlock(nwfilters->objs[i]); + obj = nwfilters->objs[i]; + virNWFilterObjLock(obj); + def = obj->def; + if (STREQ_NULLABLE(def->name, name)) + return obj; + virNWFilterObjUnlock(obj); } return NULL; @@ -125,6 +138,7 @@ _virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, size_t i; virNWFilterEntryPtr entry; virNWFilterObjPtr obj; + virNWFilterDefPtr objdef; if (!def) return 0; @@ -141,9 +155,9 @@ _virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, obj = virNWFilterObjFindByName(nwfilters, entry->include->filterref); if (obj) { - rc = _virNWFilterObjDefLoopDetect(nwfilters, - obj->def, filtername); - + objdef = obj->def; + rc = _virNWFilterObjDefLoopDetect(nwfilters, objdef, + filtername); virNWFilterObjUnlock(obj); if (rc < 0) break; @@ -226,24 +240,26 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, virNWFilterDefPtr def) { virNWFilterObjPtr obj; + virNWFilterDefPtr objdef; - obj = virNWFilterObjFindByUUID(nwfilters, def->uuid); + if ((obj = virNWFilterObjFindByUUID(nwfilters, def->uuid))) { + objdef = obj->def; - if (obj) { - if (STRNEQ(def->name, obj->def->name)) { + if (STRNEQ(def->name, objdef->name)) { virReportError(VIR_ERR_OPERATION_FAILED, _("filter with same UUID but different name " "('%s') already exists"), - obj->def->name); + objdef->name); virNWFilterObjUnlock(obj); return NULL; } virNWFilterObjUnlock(obj); } else { - obj = virNWFilterObjFindByName(nwfilters, def->name); - if (obj) { + if ((obj = virNWFilterObjFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->def->uuid, uuidstr); + + objdef = obj->def; + virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); @@ -261,8 +277,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, if ((obj = virNWFilterObjFindByName(nwfilters, def->name))) { - if (virNWFilterDefEqual(def, obj->def, false)) { - virNWFilterDefFree(obj->def); + objdef = obj->def; + if (virNWFilterDefEqual(def, objdef, false)) { + virNWFilterDefFree(objdef); obj->def = def; return obj; } @@ -275,7 +292,7 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } - virNWFilterDefFree(obj->def); + virNWFilterDefFree(objdef); obj->def = def; obj->newDef = NULL; return obj; @@ -312,11 +329,13 @@ virNWFilterObjNumOfNWFilters(virNWFilterObjListPtr nwfilters, { size_t i; int nfilters = 0; + virNWFilterDefPtr def; for (i = 0; i < nwfilters->count; i++) { virNWFilterObjPtr obj = nwfilters->objs[i]; virNWFilterObjLock(obj); - if (!aclfilter || aclfilter(conn, obj->def)) + def = obj->def; + if (!aclfilter || aclfilter(conn, def)) nfilters++; virNWFilterObjUnlock(obj); } @@ -334,12 +353,14 @@ virNWFilterObjGetNames(virNWFilterObjListPtr nwfilters, { int nnames = 0; size_t i; + virNWFilterDefPtr def; for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { virNWFilterObjPtr obj = nwfilters->objs[i]; virNWFilterObjLock(obj); - if (!aclfilter || aclfilter(conn, obj->def)) { - if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { + def = obj->def; + if (!aclfilter || aclfilter(conn, def)) { + if (VIR_STRDUP(names[nnames], def->name) < 0) { virNWFilterObjUnlock(obj); goto failure; } @@ -368,6 +389,7 @@ virNWFilterObjListExport(virConnectPtr conn, int nfilters = 0; virNWFilterPtr filter = NULL; virNWFilterObjPtr obj = NULL; + virNWFilterDefPtr def; size_t i; int ret = -1; @@ -382,9 +404,9 @@ virNWFilterObjListExport(virConnectPtr conn, for (i = 0; i < nwfilters->count; i++) { obj = nwfilters->objs[i]; virNWFilterObjLock(obj); - if (!aclfilter || aclfilter(conn, obj->def)) { - if (!(filter = virGetNWFilter(conn, obj->def->name, - obj->def->uuid))) { + def = obj->def; + if (!aclfilter || aclfilter(conn, def)) { + if (!(filter = virGetNWFilter(conn, def->name, def->uuid))) { virNWFilterObjUnlock(obj); goto cleanup; } diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 72b9b8e..7e88a40 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -358,6 +358,7 @@ nwfilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { virNWFilterObjPtr obj; + virNWFilterDefPtr def; virNWFilterPtr ret = NULL; nwfilterDriverLock(); @@ -369,11 +370,12 @@ nwfilterLookupByUUID(virConnectPtr conn, "%s", _("no nwfilter with matching uuid")); goto cleanup; } + def = obj->def; - if (virNWFilterLookupByUUIDEnsureACL(conn, obj->def) < 0) + if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0) goto cleanup; - ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid); + ret = virGetNWFilter(conn, def->name, def->uuid); cleanup: if (obj) @@ -387,6 +389,7 @@ nwfilterLookupByName(virConnectPtr conn, const char *name) { virNWFilterObjPtr obj; + virNWFilterDefPtr def; virNWFilterPtr ret = NULL; nwfilterDriverLock(); @@ -398,11 +401,12 @@ nwfilterLookupByName(virConnectPtr conn, _("no nwfilter with matching name '%s'"), name); goto cleanup; } + def = obj->def; - if (virNWFilterLookupByNameEnsureACL(conn, obj->def) < 0) + if (virNWFilterLookupByNameEnsureACL(conn, def) < 0) goto cleanup; - ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid); + ret = virGetNWFilter(conn, def->name, def->uuid); cleanup: if (obj) @@ -467,6 +471,7 @@ nwfilterDefineXML(virConnectPtr conn, { virNWFilterDefPtr def; virNWFilterObjPtr obj = NULL; + virNWFilterDefPtr objdef; virNWFilterPtr ret = NULL; if (!driver->privileged) { @@ -487,15 +492,15 @@ nwfilterDefineXML(virConnectPtr conn, if (!(obj = virNWFilterObjAssignDef(&driver->nwfilters, def))) goto cleanup; + def = NULL; + objdef = obj->def; - if (virNWFilterSaveDef(driver->configDir, def) < 0) { + if (virNWFilterSaveDef(driver->configDir, objdef) < 0) { virNWFilterObjRemove(&driver->nwfilters, obj); - def = NULL; goto cleanup; } - def = NULL; - ret = virGetNWFilter(conn, obj->def->name, obj->def->uuid); + ret = virGetNWFilter(conn, objdef->name, objdef->uuid); cleanup: virNWFilterDefFree(def); @@ -513,6 +518,7 @@ static int nwfilterUndefine(virNWFilterPtr nwfilter) { virNWFilterObjPtr obj; + virNWFilterDefPtr def; int ret = -1; nwfilterDriverLock(); @@ -525,8 +531,9 @@ nwfilterUndefine(virNWFilterPtr nwfilter) "%s", _("no nwfilter with matching uuid")); goto cleanup; } + def = obj->def; - if (virNWFilterUndefineEnsureACL(nwfilter->conn, obj->def) < 0) + if (virNWFilterUndefineEnsureACL(nwfilter->conn, def) < 0) goto cleanup; if (virNWFilterObjTestUnassignDef(obj) < 0) { @@ -536,7 +543,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; } - if (virNWFilterDeleteDef(driver->configDir, obj->def) < 0) + if (virNWFilterDeleteDef(driver->configDir, def) < 0) goto cleanup; virNWFilterObjRemove(&driver->nwfilters, obj); @@ -559,6 +566,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, unsigned int flags) { virNWFilterObjPtr obj; + virNWFilterDefPtr def; char *ret = NULL; virCheckFlags(0, NULL); @@ -572,11 +580,12 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, "%s", _("no nwfilter with matching uuid")); goto cleanup; } + def = obj->def; - if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, obj->def) < 0) + if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, def) < 0) goto cleanup; - ret = virNWFilterDefFormat(obj->def); + ret = virNWFilterDefFormat(def); cleanup: if (obj) -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:31PM -0400, John Ferlan wrote:
Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X create local virNWFilterObjPtr and virNWFilterDefPtr variables.
Future adjustments will be privatizing the object more, so this just prepares the code for that reality.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++--------------- src/nwfilter/nwfilter_driver.c | 33 ++++++++++------- 2 files changed, 72 insertions(+), 41 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index cb697f9..3c6bdbb 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj"); void virNWFilterObjFree(virNWFilterObjPtr obj) { + virNWFilterDefPtr def; + virNWFilterDefPtr newDef; + if (!obj) return; + def = obj->def; + newDef = obj->newDef;
- virNWFilterDefFree(obj->def); - virNWFilterDefFree(obj->newDef); + virNWFilterDefFree(def); + virNWFilterDefFree(newDef);
This was discussed in the secret cleanup series. In this case it just adds some lines to the code without any real benefit, so it's just a noise in this case. This change makes sense for functions where the *def* is used several times, but for those simple usages of def there is no point of having a separate variable. Now I know that some future patches may benefit from this change, however there is no guarantee that the patches will be accepted and pushed I think that you should wait with these changes for that future series. Pavel
On 04/25/2017 09:18 AM, Pavel Hrdina wrote:
On Mon, Apr 24, 2017 at 03:18:31PM -0400, John Ferlan wrote:
Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X create local virNWFilterObjPtr and virNWFilterDefPtr variables.
Future adjustments will be privatizing the object more, so this just prepares the code for that reality.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++--------------- src/nwfilter/nwfilter_driver.c | 33 ++++++++++------- 2 files changed, 72 insertions(+), 41 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index cb697f9..3c6bdbb 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj"); void virNWFilterObjFree(virNWFilterObjPtr obj) { + virNWFilterDefPtr def; + virNWFilterDefPtr newDef; + if (!obj) return; + def = obj->def; + newDef = obj->newDef;
- virNWFilterDefFree(obj->def); - virNWFilterDefFree(obj->newDef); + virNWFilterDefFree(def); + virNWFilterDefFree(newDef);
This was discussed in the secret cleanup series. In this case it just adds some lines to the code without any real benefit, so it's just a noise in this case. This change makes sense for functions where the *def* is used several times, but for those simple usages of def there is no point of having a separate variable.
Now I know that some future patches may benefit from this change, however there is no guarantee that the patches will be accepted and pushed I think that you should wait with these changes for that future series.
Pavel
I wouldn't be the first and I won't be the last to make/post changes to code to help some future yet unposted series that has the some 'issue' w/r/t a "guarantee" that it would accepted/pushed. Like I pointed out in the secrets series - stopping progress here because it's undesirable to have @def alone doesn't mean it's not technically correct and the reality is the compiler will do whatever it wants... Again, like the secrets code I can agree if it's just "obj->def", I can restore that, but once it's "obj->def->X", then having a "def" is more desirable regardless if there's only one such reference in the code. Pulling this patch from the entire series begins to impact (e.g. force me down the git merge path) starting at patch 5. So it's very painful to do. John
On Tue, Apr 25, 2017 at 11:42:37AM -0400, John Ferlan wrote:
On 04/25/2017 09:18 AM, Pavel Hrdina wrote:
On Mon, Apr 24, 2017 at 03:18:31PM -0400, John Ferlan wrote:
Rather than dereferencing obj->def->XXX or nwfilters->objs[i]->X create local virNWFilterObjPtr and virNWFilterDefPtr variables.
Future adjustments will be privatizing the object more, so this just prepares the code for that reality.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 80 +++++++++++++++++++++++++++--------------- src/nwfilter/nwfilter_driver.c | 33 ++++++++++------- 2 files changed, 72 insertions(+), 41 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index cb697f9..3c6bdbb 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -37,11 +37,16 @@ VIR_LOG_INIT("conf.virnwfilterobj"); void virNWFilterObjFree(virNWFilterObjPtr obj) { + virNWFilterDefPtr def; + virNWFilterDefPtr newDef; + if (!obj) return; + def = obj->def; + newDef = obj->newDef;
- virNWFilterDefFree(obj->def); - virNWFilterDefFree(obj->newDef); + virNWFilterDefFree(def); + virNWFilterDefFree(newDef);
This was discussed in the secret cleanup series. In this case it just adds some lines to the code without any real benefit, so it's just a noise in this case. This change makes sense for functions where the *def* is used several times, but for those simple usages of def there is no point of having a separate variable.
Now I know that some future patches may benefit from this change, however there is no guarantee that the patches will be accepted and pushed I think that you should wait with these changes for that future series.
Pavel
I wouldn't be the first and I won't be the last to make/post changes to code to help some future yet unposted series that has the some 'issue' w/r/t a "guarantee" that it would accepted/pushed.
Like I pointed out in the secrets series - stopping progress here because it's undesirable to have @def alone doesn't mean it's not technically correct and the reality is the compiler will do whatever it wants...
Again, like the secrets code I can agree if it's just "obj->def", I can restore that, but once it's "obj->def->X", then having a "def" is more desirable regardless if there's only one such reference in the code.
That sounds reasonable, I can agree on that. I don't want to stop any progress, I just didn't like the first case that will be dropped. Pavel
Pulling this patch from the entire series begins to impact (e.g. force me down the git merge path) starting at patch 5. So it's very painful to do.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
It was only ever set to false, which is ironically the default Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 1 - src/conf/virnwfilterobj.h | 1 - 2 files changed, 2 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 3c6bdbb..9e7d584 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -308,7 +308,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } virNWFilterObjLock(obj); - obj->active = 0; if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) { diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 2adffd9..7a2addf 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -29,7 +29,6 @@ typedef virNWFilterObj *virNWFilterObjPtr; struct _virNWFilterObj { virMutex lock; - int active; int wantRemoved; virNWFilterDefPtr def; -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:32PM -0400, John Ferlan wrote:
It was only ever set to false, which is ironically the default
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 1 - src/conf/virnwfilterobj.h | 1 - 2 files changed, 2 deletions(-)
ACK Pavel
It is what it is anyway, so let's describe it that way too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 4 ++-- src/conf/virnwfilterobj.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 9e7d584..6e5c5b7 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -192,12 +192,12 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj) { int rc = 0; - obj->wantRemoved = 1; + obj->wantRemoved = true; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild()) rc = -1; - obj->wantRemoved = 0; + obj->wantRemoved = false; return rc; } diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 7a2addf..f31938d 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -29,7 +29,7 @@ typedef virNWFilterObj *virNWFilterObjPtr; struct _virNWFilterObj { virMutex lock; - int wantRemoved; + bool wantRemoved; virNWFilterDefPtr def; virNWFilterDefPtr newDef; -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:33PM -0400, John Ferlan wrote:
It is what it is anyway, so let's describe it that way too.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 4 ++-- src/conf/virnwfilterobj.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
ACK Pavel
Move the structure to virnwfilterobj.c and create necessary accessor API's for the various fields. Also make virNWFilterObjFree static since there's no external callers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 32 +++++++++++++++++++++++++++++++- src/conf/virnwfilterobj.h | 22 +++++++++------------- src/libvirt_private.syms | 3 +++ src/nwfilter/nwfilter_driver.c | 10 +++++----- src/nwfilter/nwfilter_gentech_driver.c | 30 ++++++++++++++++++------------ 5 files changed, 66 insertions(+), 31 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 6e5c5b7..9846751 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -33,8 +33,38 @@ VIR_LOG_INIT("conf.virnwfilterobj"); +struct _virNWFilterObj { + virMutex lock; -void + bool wantRemoved; + + virNWFilterDefPtr def; + virNWFilterDefPtr newDef; +}; + + +virNWFilterDefPtr +virNWFilterObjGetDef(virNWFilterObjPtr obj) +{ + return obj->def; +} + + +virNWFilterDefPtr +virNWFilterObjGetNewDef(virNWFilterObjPtr obj) +{ + return obj->newDef; +} + + +bool +virNWFilterObjWantRemoved(virNWFilterObjPtr obj) +{ + return obj->wantRemoved; +} + + +static void virNWFilterObjFree(virNWFilterObjPtr obj) { virNWFilterDefPtr def; diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index f31938d..b02dcfa 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -26,16 +26,6 @@ typedef struct _virNWFilterObj virNWFilterObj; typedef virNWFilterObj *virNWFilterObjPtr; -struct _virNWFilterObj { - virMutex lock; - - bool wantRemoved; - - virNWFilterDefPtr def; - virNWFilterDefPtr newDef; -}; - - typedef struct _virNWFilterObjList virNWFilterObjList; typedef virNWFilterObjList *virNWFilterObjListPtr; struct _virNWFilterObjList { @@ -56,6 +46,15 @@ struct _virNWFilterDriverState { bool watchingFirewallD; }; +virNWFilterDefPtr +virNWFilterObjGetDef(virNWFilterObjPtr obj); + +virNWFilterDefPtr +virNWFilterObjGetNewDef(virNWFilterObjPtr obj); + +bool +virNWFilterObjWantRemoved(virNWFilterObjPtr obj); + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters); @@ -63,9 +62,6 @@ void virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj); -void -virNWFilterObjFree(virNWFilterObjPtr obj); - virNWFilterObjPtr virNWFilterObjFindByUUID(virNWFilterObjListPtr nwfilters, const unsigned char *uuid); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 83e979a..dd6cb98 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -965,7 +965,9 @@ virNodeDeviceObjUnlock; virNWFilterObjAssignDef; virNWFilterObjFindByName; virNWFilterObjFindByUUID; +virNWFilterObjGetDef; virNWFilterObjGetNames; +virNWFilterObjGetNewDef; virNWFilterObjListExport; virNWFilterObjListFree; virNWFilterObjLoadAllConfigs; @@ -974,6 +976,7 @@ virNWFilterObjNumOfNWFilters; virNWFilterObjRemove; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; +virNWFilterObjWantRemoved; # conf/virsecretobj.h diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 7e88a40..dd3645a 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -370,7 +370,7 @@ nwfilterLookupByUUID(virConnectPtr conn, "%s", _("no nwfilter with matching uuid")); goto cleanup; } - def = obj->def; + def = virNWFilterObjGetDef(obj); if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0) goto cleanup; @@ -401,7 +401,7 @@ nwfilterLookupByName(virConnectPtr conn, _("no nwfilter with matching name '%s'"), name); goto cleanup; } - def = obj->def; + def = virNWFilterObjGetDef(obj); if (virNWFilterLookupByNameEnsureACL(conn, def) < 0) goto cleanup; @@ -493,7 +493,7 @@ nwfilterDefineXML(virConnectPtr conn, if (!(obj = virNWFilterObjAssignDef(&driver->nwfilters, def))) goto cleanup; def = NULL; - objdef = obj->def; + objdef = virNWFilterObjGetDef(obj); if (virNWFilterSaveDef(driver->configDir, objdef) < 0) { virNWFilterObjRemove(&driver->nwfilters, obj); @@ -531,7 +531,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) "%s", _("no nwfilter with matching uuid")); goto cleanup; } - def = obj->def; + def = virNWFilterObjGetDef(obj); if (virNWFilterUndefineEnsureACL(nwfilter->conn, def) < 0) goto cleanup; @@ -580,7 +580,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, "%s", _("no nwfilter with matching uuid")); goto cleanup; } - def = obj->def; + def = virNWFilterObjGetDef(obj); if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, def) < 0) goto cleanup; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 761a01b..b356d87 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -379,6 +379,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, virNWFilterObjPtr obj; virNWFilterHashTablePtr tmpvars = NULL; virNWFilterDefPtr childdef; + virNWFilterDefPtr newChilddef; int ret = -1; VIR_DEBUG("Instantiating filter %s", inc->filterref); @@ -390,7 +391,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, inc->filterref); goto cleanup; } - if (obj->wantRemoved) { + if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), inc->filterref); @@ -402,12 +403,13 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, vars))) goto cleanup; - childdef = obj->def; + childdef = virNWFilterObjGetDef(obj); switch (useNewFilter) { case INSTANTIATE_FOLLOW_NEWFILTER: - if (obj->newDef) { - childdef = obj->newDef; + newChilddef = virNWFilterObjGetNewDef(obj); + if (newChilddef) { + childdef = newChilddef; *foundNewFilter = true; } break; @@ -505,6 +507,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, int rc = 0; size_t i, j; virNWFilterDefPtr next_filter; + virNWFilterDefPtr newNext_filter; virNWFilterVarValuePtr val; for (i = 0; i < filter->nentries; i++) { @@ -545,7 +548,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, obj = virNWFilterObjFindByName(&driver->nwfilters, inc->filterref); if (obj) { - if (obj->wantRemoved) { + if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), inc->filterref); @@ -564,12 +567,13 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, break; } - next_filter = obj->def; + next_filter = virNWFilterObjGetDef(obj); switch (useNewFilter) { case INSTANTIATE_FOLLOW_NEWFILTER: - if (obj->newDef) - next_filter = obj->newDef; + newNext_filter = virNWFilterObjGetNewDef(obj); + if (newNext_filter) + next_filter = newNext_filter; break; case INSTANTIATE_ALWAYS: break; @@ -790,6 +794,7 @@ __virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, virNWFilterObjPtr obj; virNWFilterHashTablePtr vars, vars1; virNWFilterDefPtr filter; + virNWFilterDefPtr newFilter; char vmmacaddr[VIR_MAC_STRING_BUFLEN] = {0}; char *str_macaddr = NULL; virNWFilterVarValuePtr ipaddr; @@ -815,7 +820,7 @@ __virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, return -1; } - if (obj->wantRemoved) { + if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); @@ -847,12 +852,13 @@ __virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, goto err_exit_vars1; } - filter = obj->def; + filter = virNWFilterObjGetDef(obj); switch (useNewFilter) { case INSTANTIATE_FOLLOW_NEWFILTER: - if (obj->newDef) { - filter = obj->newDef; + newFilter = virNWFilterObjGetNewDef(obj); + if (newFilter) { + filter = newFilter; *foundNewFilter = true; } break; -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:34PM -0400, John Ferlan wrote:
Move the structure to virnwfilterobj.c and create necessary accessor API's for the various fields.
Also make virNWFilterObjFree static since there's no external callers.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 32 +++++++++++++++++++++++++++++++- src/conf/virnwfilterobj.h | 22 +++++++++------------- src/libvirt_private.syms | 3 +++ src/nwfilter/nwfilter_driver.c | 10 +++++----- src/nwfilter/nwfilter_gentech_driver.c | 30 ++++++++++++++++++------------ 5 files changed, 66 insertions(+), 31 deletions(-)
ACK Pavel
Perform the object initialization in a helper rather than inline Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 9846751..dcfd1e7 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -43,6 +43,26 @@ struct _virNWFilterObj { }; +static virNWFilterObjPtr +virNWFilterObjNew(void) +{ + virNWFilterObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInitRecursive(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + + virNWFilterObjLock(obj); + return obj; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -328,16 +348,8 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, return obj; } - if (VIR_ALLOC(obj) < 0) - return NULL; - - if (virMutexInitRecursive(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virNWFilterObjNew())) return NULL; - } - virNWFilterObjLock(obj); if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) { -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:35PM -0400, John Ferlan wrote:
Perform the object initialization in a helper rather than inline
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
ACK Pavel
Prefix should have been virNWFilterObjList since the API is operating on the list of filters Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 76 +++++++++++++++++----------------- src/conf/virnwfilterobj.h | 36 ++++++++-------- src/libvirt_private.syms | 14 +++---- src/nwfilter/nwfilter_driver.c | 22 +++++----- src/nwfilter/nwfilter_gentech_driver.c | 12 +++--- 5 files changed, 80 insertions(+), 80 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index dcfd1e7..a259062 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -116,8 +116,8 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) void -virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, - virNWFilterObjPtr obj) +virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj) { size_t i; @@ -138,8 +138,8 @@ virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr -virNWFilterObjFindByUUID(virNWFilterObjListPtr nwfilters, - const unsigned char *uuid) +virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, + const unsigned char *uuid) { size_t i; virNWFilterObjPtr obj; @@ -159,8 +159,8 @@ virNWFilterObjFindByUUID(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr -virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters, - const char *name) +virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, + const char *name) { size_t i; virNWFilterObjPtr obj; @@ -180,9 +180,9 @@ virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters, static int -_virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, - virNWFilterDefPtr def, - const char *filtername) +_virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, + virNWFilterDefPtr def, + const char *filtername) { int rc = 0; size_t i; @@ -202,12 +202,12 @@ _virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, break; } - obj = virNWFilterObjFindByName(nwfilters, - entry->include->filterref); + obj = virNWFilterObjListFindByName(nwfilters, + entry->include->filterref); if (obj) { objdef = obj->def; - rc = _virNWFilterObjDefLoopDetect(nwfilters, objdef, - filtername); + rc = _virNWFilterObjListDefLoopDetect(nwfilters, objdef, + filtername); virNWFilterObjUnlock(obj); if (rc < 0) break; @@ -220,7 +220,7 @@ _virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, /* - * virNWFilterObjDefLoopDetect: + * virNWFilterObjListDefLoopDetect: * @nwfilters : the nwfilters to search * @def : the filter definition that may add a loop and is to be tested * @@ -230,10 +230,10 @@ _virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, * Returns 0 in case no loop was detected, -1 otherwise. */ static int -virNWFilterObjDefLoopDetect(virNWFilterObjListPtr nwfilters, - virNWFilterDefPtr def) +virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, + virNWFilterDefPtr def) { - return _virNWFilterObjDefLoopDetect(nwfilters, def, def->name); + return _virNWFilterObjListDefLoopDetect(nwfilters, def, def->name); } @@ -286,13 +286,13 @@ virNWFilterDefEqual(const virNWFilterDef *def1, virNWFilterObjPtr -virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, - virNWFilterDefPtr def) +virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, + virNWFilterDefPtr def) { virNWFilterObjPtr obj; virNWFilterDefPtr objdef; - if ((obj = virNWFilterObjFindByUUID(nwfilters, def->uuid))) { + if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) { objdef = obj->def; if (STRNEQ(def->name, objdef->name)) { @@ -305,7 +305,7 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, } virNWFilterObjUnlock(obj); } else { - if ((obj = virNWFilterObjFindByName(nwfilters, def->name))) { + if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; objdef = obj->def; @@ -318,14 +318,14 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, } } - if (virNWFilterObjDefLoopDetect(nwfilters, def) < 0) { + if (virNWFilterObjListDefLoopDetect(nwfilters, def) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("filter would introduce a loop")); return NULL; } - if ((obj = virNWFilterObjFindByName(nwfilters, def->name))) { + if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { objdef = obj->def; if (virNWFilterDefEqual(def, objdef, false)) { @@ -364,9 +364,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, int -virNWFilterObjNumOfNWFilters(virNWFilterObjListPtr nwfilters, - virConnectPtr conn, - virNWFilterObjListFilter aclfilter) +virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, + virConnectPtr conn, + virNWFilterObjListFilter aclfilter) { size_t i; int nfilters = 0; @@ -386,11 +386,11 @@ virNWFilterObjNumOfNWFilters(virNWFilterObjListPtr nwfilters, int -virNWFilterObjGetNames(virNWFilterObjListPtr nwfilters, - virConnectPtr conn, - virNWFilterObjListFilter aclfilter, - char **const names, - int maxnames) +virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, + virConnectPtr conn, + virNWFilterObjListFilter aclfilter, + char **const names, + int maxnames) { int nnames = 0; size_t i; @@ -472,9 +472,9 @@ virNWFilterObjListExport(virConnectPtr conn, static virNWFilterObjPtr -virNWFilterObjLoadConfig(virNWFilterObjListPtr nwfilters, - const char *configDir, - const char *name) +virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, + const char *configDir, + const char *name) { virNWFilterDefPtr def = NULL; virNWFilterObjPtr obj; @@ -499,7 +499,7 @@ virNWFilterObjLoadConfig(virNWFilterObjListPtr nwfilters, virNWFilterSaveConfig(configDir, def) < 0) goto error; - if (!(obj = virNWFilterObjAssignDef(nwfilters, def))) + if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) goto error; VIR_FREE(configFile); @@ -513,8 +513,8 @@ virNWFilterObjLoadConfig(virNWFilterObjListPtr nwfilters, int -virNWFilterObjLoadAllConfigs(virNWFilterObjListPtr nwfilters, - const char *configDir) +virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, + const char *configDir) { DIR *dir; struct dirent *entry; @@ -530,7 +530,7 @@ virNWFilterObjLoadAllConfigs(virNWFilterObjListPtr nwfilters, if (!virFileStripSuffix(entry->d_name, ".xml")) continue; - obj = virNWFilterObjLoadConfig(nwfilters, configDir, entry->d_name); + obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name); if (obj) virNWFilterObjUnlock(obj); } diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index b02dcfa..4c19223 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -59,20 +59,20 @@ void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters); void -virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, - virNWFilterObjPtr obj); +virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, + virNWFilterObjPtr obj); virNWFilterObjPtr -virNWFilterObjFindByUUID(virNWFilterObjListPtr nwfilters, - const unsigned char *uuid); +virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, + const unsigned char *uuid); virNWFilterObjPtr -virNWFilterObjFindByName(virNWFilterObjListPtr nwfilters, - const char *name); +virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, + const char *name); virNWFilterObjPtr -virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters, - virNWFilterDefPtr def); +virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, + virNWFilterDefPtr def); int virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj); @@ -82,16 +82,16 @@ typedef bool virNWFilterDefPtr def); int -virNWFilterObjNumOfNWFilters(virNWFilterObjListPtr nwfilters, - virConnectPtr conn, - virNWFilterObjListFilter aclfilter); +virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, + virConnectPtr conn, + virNWFilterObjListFilter aclfilter); int -virNWFilterObjGetNames(virNWFilterObjListPtr nwfilters, - virConnectPtr conn, - virNWFilterObjListFilter aclfilter, - char **const names, - int maxnames); +virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, + virConnectPtr conn, + virNWFilterObjListFilter aclfilter, + char **const names, + int maxnames); int virNWFilterObjListExport(virConnectPtr conn, @@ -100,8 +100,8 @@ virNWFilterObjListExport(virConnectPtr conn, virNWFilterObjListFilter aclfilter); int -virNWFilterObjLoadAllConfigs(virNWFilterObjListPtr nwfilters, - const char *configDir); +virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, + const char *configDir); void virNWFilterObjLock(virNWFilterObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd6cb98..453d0fe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -962,18 +962,18 @@ virNodeDeviceObjUnlock; # conf/virnwfilterobj.h -virNWFilterObjAssignDef; -virNWFilterObjFindByName; -virNWFilterObjFindByUUID; virNWFilterObjGetDef; -virNWFilterObjGetNames; virNWFilterObjGetNewDef; +virNWFilterObjListAssignDef; virNWFilterObjListExport; +virNWFilterObjListFindByName; +virNWFilterObjListFindByUUID; virNWFilterObjListFree; -virNWFilterObjLoadAllConfigs; +virNWFilterObjListGetNames; +virNWFilterObjListLoadAllConfigs; +virNWFilterObjListNumOfNWFilters; +virNWFilterObjListRemove; virNWFilterObjLock; -virNWFilterObjNumOfNWFilters; -virNWFilterObjRemove; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index dd3645a..6516053 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -237,7 +237,7 @@ nwfilterStateInitialize(bool privileged, VIR_FREE(base); - if (virNWFilterObjLoadAllConfigs(&driver->nwfilters, driver->configDir) < 0) + if (virNWFilterObjListLoadAllConfigs(&driver->nwfilters, driver->configDir) < 0) goto error; nwfilterDriverUnlock(); @@ -289,7 +289,7 @@ nwfilterStateReload(void) virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - virNWFilterObjLoadAllConfigs(&driver->nwfilters, driver->configDir); + virNWFilterObjListLoadAllConfigs(&driver->nwfilters, driver->configDir); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -362,7 +362,7 @@ nwfilterLookupByUUID(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(); - obj = virNWFilterObjFindByUUID(&driver->nwfilters, uuid); + obj = virNWFilterObjListFindByUUID(&driver->nwfilters, uuid); nwfilterDriverUnlock(); if (!obj) { @@ -393,7 +393,7 @@ nwfilterLookupByName(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(); - obj = virNWFilterObjFindByName(&driver->nwfilters, name); + obj = virNWFilterObjListFindByName(&driver->nwfilters, name); nwfilterDriverUnlock(); if (!obj) { @@ -421,7 +421,7 @@ nwfilterConnectNumOfNWFilters(virConnectPtr conn) if (virConnectNumOfNWFiltersEnsureACL(conn) < 0) return -1; - return virNWFilterObjNumOfNWFilters(&driver->nwfilters, conn, + return virNWFilterObjListNumOfNWFilters(&driver->nwfilters, conn, virConnectNumOfNWFiltersCheckACL); } @@ -437,7 +437,7 @@ nwfilterConnectListNWFilters(virConnectPtr conn, return -1; nwfilterDriverLock(); - nnames = virNWFilterObjGetNames(&driver->nwfilters, conn, + nnames = virNWFilterObjListGetNames(&driver->nwfilters, conn, virConnectListNWFiltersCheckACL, names, maxnames); nwfilterDriverUnlock(); @@ -490,13 +490,13 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterDefineXMLEnsureACL(conn, def) < 0) goto cleanup; - if (!(obj = virNWFilterObjAssignDef(&driver->nwfilters, def))) + if (!(obj = virNWFilterObjListAssignDef(&driver->nwfilters, def))) goto cleanup; def = NULL; objdef = virNWFilterObjGetDef(obj); if (virNWFilterSaveDef(driver->configDir, objdef) < 0) { - virNWFilterObjRemove(&driver->nwfilters, obj); + virNWFilterObjListRemove(&driver->nwfilters, obj); goto cleanup; } @@ -525,7 +525,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - obj = virNWFilterObjFindByUUID(&driver->nwfilters, nwfilter->uuid); + obj = virNWFilterObjListFindByUUID(&driver->nwfilters, nwfilter->uuid); if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, "%s", _("no nwfilter with matching uuid")); @@ -546,7 +546,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) if (virNWFilterDeleteDef(driver->configDir, def) < 0) goto cleanup; - virNWFilterObjRemove(&driver->nwfilters, obj); + virNWFilterObjListRemove(&driver->nwfilters, obj); obj = NULL; ret = 0; @@ -572,7 +572,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, virCheckFlags(0, NULL); nwfilterDriverLock(); - obj = virNWFilterObjFindByUUID(&driver->nwfilters, nwfilter->uuid); + obj = virNWFilterObjListFindByUUID(&driver->nwfilters, nwfilter->uuid); nwfilterDriverUnlock(); if (!obj) { diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index b356d87..23f1999 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -60,11 +60,11 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter * will hold a lock on a virNWFilterObjPtr. This in turn invokes * virNWFilterInstantiate which invokes virNWFilterDetermineMissingVarsRec - * which invokes virNWFilterObjFindByName. This iterates over every single + * which invokes virNWFilterObjListFindByName. This iterates over every single * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a * filter in parallel, they'll both hold 1 lock at the top level in * __virNWFilterInstantiateFilter which will cause the other thread - * to deadlock in virNWFilterObjFindByName. + * to deadlock in virNWFilterObjListFindByName. * * XXX better long term solution is to make virNWFilterObjList use a * hash table as is done for virDomainObjList. You can then get @@ -383,8 +383,8 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, int ret = -1; VIR_DEBUG("Instantiating filter %s", inc->filterref); - obj = virNWFilterObjFindByName(&driver->nwfilters, - inc->filterref); + obj = virNWFilterObjListFindByName(&driver->nwfilters, + inc->filterref); if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), @@ -545,7 +545,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, break; } else if (inc) { VIR_DEBUG("Following filter %s", inc->filterref); - obj = virNWFilterObjFindByName(&driver->nwfilters, inc->filterref); + obj = virNWFilterObjListFindByName(&driver->nwfilters, inc->filterref); if (obj) { if (virNWFilterObjWantRemoved(obj)) { @@ -812,7 +812,7 @@ __virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, VIR_DEBUG("filter name: %s", filtername); - obj = virNWFilterObjFindByName(&driver->nwfilters, filtername); + obj = virNWFilterObjListFindByName(&driver->nwfilters, filtername); if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, _("Could not find filter '%s'"), -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:36PM -0400, John Ferlan wrote:
Prefix should have been virNWFilterObjList since the API is operating on the list of filters
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 76 +++++++++++++++++----------------- src/conf/virnwfilterobj.h | 36 ++++++++-------- src/libvirt_private.syms | 14 +++---- src/nwfilter/nwfilter_driver.c | 22 +++++----- src/nwfilter/nwfilter_gentech_driver.c | 12 +++--- 5 files changed, 80 insertions(+), 80 deletions(-)
ACK Pavel
Move from virnwfilterobj.h to virnwfilterobj.c. Create the virNWFilterObjListNew() API in order to allocate Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 16 ++++++++++++++++ src/conf/virnwfilterobj.h | 10 ++++------ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_driver.c | 29 ++++++++++++++++------------- src/nwfilter/nwfilter_gentech_driver.c | 6 +++--- 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index a259062..7410047 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -42,6 +42,11 @@ struct _virNWFilterObj { virNWFilterDefPtr newDef; }; +struct _virNWFilterObjList { + size_t count; + virNWFilterObjPtr *objs; +}; + static virNWFilterObjPtr virNWFilterObjNew(void) @@ -115,6 +120,17 @@ virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) } +virNWFilterObjListPtr +virNWFilterObjListNew(void) +{ + virNWFilterObjListPtr nwfilters; + + if (VIR_ALLOC(nwfilters) < 0) + return NULL; + return nwfilters; +} + + void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj) diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 4c19223..29d9086 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -28,11 +28,6 @@ typedef virNWFilterObj *virNWFilterObjPtr; typedef struct _virNWFilterObjList virNWFilterObjList; typedef virNWFilterObjList *virNWFilterObjListPtr; -struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; -}; - typedef struct _virNWFilterDriverState virNWFilterDriverState; typedef virNWFilterDriverState *virNWFilterDriverStatePtr; @@ -40,7 +35,7 @@ struct _virNWFilterDriverState { virMutex lock; bool privileged; - virNWFilterObjList nwfilters; + virNWFilterObjListPtr nwfilters; char *configDir; bool watchingFirewallD; @@ -55,6 +50,9 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj); bool virNWFilterObjWantRemoved(virNWFilterObjPtr obj); +virNWFilterObjListPtr +virNWFilterObjListNew(void); + void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 453d0fe..13f2ab9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -971,6 +971,7 @@ virNWFilterObjListFindByUUID; virNWFilterObjListFree; virNWFilterObjListGetNames; virNWFilterObjListLoadAllConfigs; +virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; virNWFilterObjLock; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 6516053..650c528 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -237,7 +237,10 @@ nwfilterStateInitialize(bool privileged, VIR_FREE(base); - if (virNWFilterObjListLoadAllConfigs(&driver->nwfilters, driver->configDir) < 0) + if (!(driver->nwfilters = virNWFilterObjListNew())) + goto error; + + if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir) < 0) goto error; nwfilterDriverUnlock(); @@ -289,7 +292,7 @@ nwfilterStateReload(void) virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - virNWFilterObjListLoadAllConfigs(&driver->nwfilters, driver->configDir); + virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -340,7 +343,7 @@ nwfilterStateCleanup(void) nwfilterDriverRemoveDBusMatches(); /* free inactive nwfilters */ - virNWFilterObjListFree(&driver->nwfilters); + virNWFilterObjListFree(driver->nwfilters); VIR_FREE(driver->configDir); nwfilterDriverUnlock(); @@ -362,7 +365,7 @@ nwfilterLookupByUUID(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(); - obj = virNWFilterObjListFindByUUID(&driver->nwfilters, uuid); + obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuid); nwfilterDriverUnlock(); if (!obj) { @@ -393,7 +396,7 @@ nwfilterLookupByName(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(); - obj = virNWFilterObjListFindByName(&driver->nwfilters, name); + obj = virNWFilterObjListFindByName(driver->nwfilters, name); nwfilterDriverUnlock(); if (!obj) { @@ -421,7 +424,7 @@ nwfilterConnectNumOfNWFilters(virConnectPtr conn) if (virConnectNumOfNWFiltersEnsureACL(conn) < 0) return -1; - return virNWFilterObjListNumOfNWFilters(&driver->nwfilters, conn, + return virNWFilterObjListNumOfNWFilters(driver->nwfilters, conn, virConnectNumOfNWFiltersCheckACL); } @@ -437,7 +440,7 @@ nwfilterConnectListNWFilters(virConnectPtr conn, return -1; nwfilterDriverLock(); - nnames = virNWFilterObjListGetNames(&driver->nwfilters, conn, + nnames = virNWFilterObjListGetNames(driver->nwfilters, conn, virConnectListNWFiltersCheckACL, names, maxnames); nwfilterDriverUnlock(); @@ -458,7 +461,7 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn, return -1; nwfilterDriverLock(); - ret = virNWFilterObjListExport(conn, &driver->nwfilters, filters, + ret = virNWFilterObjListExport(conn, driver->nwfilters, filters, virConnectListAllNWFiltersCheckACL); nwfilterDriverUnlock(); @@ -490,13 +493,13 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterDefineXMLEnsureACL(conn, def) < 0) goto cleanup; - if (!(obj = virNWFilterObjListAssignDef(&driver->nwfilters, def))) + if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def))) goto cleanup; def = NULL; objdef = virNWFilterObjGetDef(obj); if (virNWFilterSaveDef(driver->configDir, objdef) < 0) { - virNWFilterObjListRemove(&driver->nwfilters, obj); + virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; } @@ -525,7 +528,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - obj = virNWFilterObjListFindByUUID(&driver->nwfilters, nwfilter->uuid); + obj = virNWFilterObjListFindByUUID(driver->nwfilters, nwfilter->uuid); if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, "%s", _("no nwfilter with matching uuid")); @@ -546,7 +549,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) if (virNWFilterDeleteDef(driver->configDir, def) < 0) goto cleanup; - virNWFilterObjListRemove(&driver->nwfilters, obj); + virNWFilterObjListRemove(driver->nwfilters, obj); obj = NULL; ret = 0; @@ -572,7 +575,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, virCheckFlags(0, NULL); nwfilterDriverLock(); - obj = virNWFilterObjListFindByUUID(&driver->nwfilters, nwfilter->uuid); + obj = virNWFilterObjListFindByUUID(driver->nwfilters, nwfilter->uuid); nwfilterDriverUnlock(); if (!obj) { diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 23f1999..82e20de 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -383,7 +383,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, int ret = -1; VIR_DEBUG("Instantiating filter %s", inc->filterref); - obj = virNWFilterObjListFindByName(&driver->nwfilters, + obj = virNWFilterObjListFindByName(driver->nwfilters, inc->filterref); if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -545,7 +545,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, break; } else if (inc) { VIR_DEBUG("Following filter %s", inc->filterref); - obj = virNWFilterObjListFindByName(&driver->nwfilters, inc->filterref); + obj = virNWFilterObjListFindByName(driver->nwfilters, inc->filterref); if (obj) { if (virNWFilterObjWantRemoved(obj)) { @@ -812,7 +812,7 @@ __virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, VIR_DEBUG("filter name: %s", filtername); - obj = virNWFilterObjListFindByName(&driver->nwfilters, filtername); + obj = virNWFilterObjListFindByName(driver->nwfilters, filtername); if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, _("Could not find filter '%s'"), -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:37PM -0400, John Ferlan wrote:
Move from virnwfilterobj.h to virnwfilterobj.c.
Create the virNWFilterObjListNew() API in order to allocate
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 16 ++++++++++++++++ src/conf/virnwfilterobj.h | 10 ++++------ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_driver.c | 29 ++++++++++++++++------------- src/nwfilter/nwfilter_gentech_driver.c | 6 +++--- 5 files changed, 40 insertions(+), 22 deletions(-)
ACK Pavel
Rather than separate calls, use a common call and generate a better error message which includes the incorrect uuidstr Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 650c528..781a7a0 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -356,6 +356,21 @@ nwfilterStateCleanup(void) } +static virNWFilterObjPtr +nwfilterObjFromNWFilter(const unsigned char *uuid) +{ + virNWFilterObjPtr obj; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuid))) { + virUUIDFormat(uuid, uuidstr); + virReportError(VIR_ERR_NO_NWFILTER, + _("no nwfilter with matching uuid '%s'"), uuidstr); + } + return obj; +} + + static virNWFilterPtr nwfilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) @@ -365,14 +380,11 @@ nwfilterLookupByUUID(virConnectPtr conn, virNWFilterPtr ret = NULL; nwfilterDriverLock(); - obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuid); + obj = nwfilterObjFromNWFilter(uuid); nwfilterDriverUnlock(); - if (!obj) { - virReportError(VIR_ERR_NO_NWFILTER, - "%s", _("no nwfilter with matching uuid")); + if (!obj) goto cleanup; - } def = virNWFilterObjGetDef(obj); if (virNWFilterLookupByUUIDEnsureACL(conn, def) < 0) @@ -528,12 +540,8 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); - obj = virNWFilterObjListFindByUUID(driver->nwfilters, nwfilter->uuid); - if (!obj) { - virReportError(VIR_ERR_NO_NWFILTER, - "%s", _("no nwfilter with matching uuid")); + if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) goto cleanup; - } def = virNWFilterObjGetDef(obj); if (virNWFilterUndefineEnsureACL(nwfilter->conn, def) < 0) @@ -575,14 +583,11 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, virCheckFlags(0, NULL); nwfilterDriverLock(); - obj = virNWFilterObjListFindByUUID(driver->nwfilters, nwfilter->uuid); + obj = nwfilterObjFromNWFilter(nwfilter->uuid); nwfilterDriverUnlock(); - if (!obj) { - virReportError(VIR_ERR_NO_NWFILTER, - "%s", _("no nwfilter with matching uuid")); + if (!obj) goto cleanup; - } def = virNWFilterObjGetDef(obj); if (virNWFilterGetXMLDescEnsureACL(nwfilter->conn, def) < 0) -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:38PM -0400, John Ferlan wrote:
Rather than separate calls, use a common call and generate a better error message which includes the incorrect uuidstr
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_driver.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)
ACK Pavel
Remove open coded helper. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 13 +------------ src/conf/nwfilter_conf.h | 4 ---- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index b69d9db..2352e60 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2775,7 +2775,7 @@ virNWFilterSaveXML(const char *configDir, char *configFile = NULL; int ret = -1; - if ((configFile = virNWFilterConfigFile(configDir, def->name)) == NULL) + if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) goto cleanup; if (virFileMakePath(configDir) < 0) { @@ -3236,17 +3236,6 @@ virNWFilterDefFormat(const virNWFilterDef *def) } -char * -virNWFilterConfigFile(const char *dir, - const char *name) -{ - char *ret = NULL; - - ignore_value(virAsprintf(&ret, "%s/%s.xml", dir, name)); - return ret; -} - - int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque) diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index d8a5daf..5bf9c3d 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -593,10 +593,6 @@ int virNWFilterSaveConfig(const char *configDir, virNWFilterDefPtr def); -char * -virNWFilterConfigFile(const char *dir, - const char *name); - virNWFilterDefPtr virNWFilterDefParseString(const char *xml); -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:39PM -0400, John Ferlan wrote:
Remove open coded helper.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 13 +------------ src/conf/nwfilter_conf.h | 4 ---- 2 files changed, 1 insertion(+), 16 deletions(-)
ACK Pavel
Essentially virNWFilterSaveDef executed in a different order the same sequence of calls, so let's just make one point of reference. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 37 ------------------------------------- src/conf/nwfilter_conf.h | 4 ---- src/libvirt_private.syms | 2 +- src/nwfilter/nwfilter_driver.c | 2 +- 4 files changed, 2 insertions(+), 43 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 2352e60..752d4e1 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2932,43 +2932,6 @@ virNWFilterTriggerVMFilterRebuild(void) int -virNWFilterSaveDef(const char *configDir, - virNWFilterDefPtr def) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *xml; - int ret = -1; - char *configFile = NULL; - - if (virFileMakePath(configDir) < 0) { - virReportSystemError(errno, - _("cannot create config directory %s"), - configDir); - goto error; - } - - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto error; - - if (!(xml = virNWFilterDefFormat(def))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to generate XML")); - goto error; - } - - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "nwfilter-edit", xml); - VIR_FREE(xml); - - error: - VIR_FREE(configFile); - return ret; -} - - -int virNWFilterDeleteDef(const char *configDir, virNWFilterDefPtr def) { diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5bf9c3d..5cac260 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -570,10 +570,6 @@ int virNWFilterTriggerVMFilterRebuild(void); int -virNWFilterSaveDef(const char *configDir, - virNWFilterDefPtr def); - -int virNWFilterDeleteDef(const char *configDir, virNWFilterDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13f2ab9..170ecce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -740,7 +740,7 @@ virNWFilterRuleIsProtocolEthernet; virNWFilterRuleIsProtocolIPv4; virNWFilterRuleIsProtocolIPv6; virNWFilterRuleProtocolTypeToString; -virNWFilterSaveDef; +virNWFilterSaveConfig; virNWFilterTriggerVMFilterRebuild; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 781a7a0..faa4fe8 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -510,7 +510,7 @@ nwfilterDefineXML(virConnectPtr conn, def = NULL; objdef = virNWFilterObjGetDef(obj); - if (virNWFilterSaveDef(driver->configDir, objdef) < 0) { + if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; } -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:40PM -0400, John Ferlan wrote:
Essentially virNWFilterSaveDef executed in a different order the same sequence of calls, so let's just make one point of reference.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 37 ------------------------------------- src/conf/nwfilter_conf.h | 4 ---- src/libvirt_private.syms | 2 +- src/nwfilter/nwfilter_driver.c | 2 +- 4 files changed, 2 insertions(+), 43 deletions(-)
Nice cleanup. ACK Pavel
Rather than "wait" for the first config file to be created, force creation of the configDir during driver state initialization. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 7 ------- src/nwfilter/nwfilter_driver.c | 7 +++++++ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 752d4e1..032700c 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2778,13 +2778,6 @@ virNWFilterSaveXML(const char *configDir, if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) goto cleanup; - if (virFileMakePath(configDir) < 0) { - virReportSystemError(errno, - _("cannot create config directory '%s'"), - configDir); - goto cleanup; - } - virUUIDFormat(def->uuid, uuidstr); ret = virXMLSaveFile(configFile, virXMLPickShellSafeComment(def->name, uuidstr), diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index faa4fe8..5e62023 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -40,6 +40,7 @@ #include "nwfilter_driver.h" #include "nwfilter_gentech_driver.h" #include "configmake.h" +#include "virfile.h" #include "virstring.h" #include "viraccessapicheck.h" @@ -237,6 +238,12 @@ nwfilterStateInitialize(bool privileged, VIR_FREE(base); + if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create config directory '%s'"), + driver->configDir); + goto error; + } + if (!(driver->nwfilters = virNWFilterObjListNew())) goto error; -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:41PM -0400, John Ferlan wrote:
Rather than "wait" for the first config file to be created, force creation of the configDir during driver state initialization.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 7 ------- src/nwfilter/nwfilter_driver.c | 7 +++++++ 2 files changed, 7 insertions(+), 7 deletions(-)
ACK Pavel
In preparation for it becoming part of the nwfilter object, move all the processing of the generation of the configFile name into the driver code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 36 +++++++++--------------------------- src/conf/nwfilter_conf.h | 6 +++--- src/conf/virnwfilterobj.c | 2 +- src/nwfilter/nwfilter_driver.c | 14 ++++++++++++-- 4 files changed, 25 insertions(+), 33 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 032700c..6b67c3a 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2767,30 +2767,21 @@ virNWFilterDefParseFile(const char *filename) int -virNWFilterSaveXML(const char *configDir, +virNWFilterSaveXML(const char *configFile, virNWFilterDefPtr def, const char *xml) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL; - int ret = -1; - - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto cleanup; virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "nwfilter-edit", xml); - - cleanup: - VIR_FREE(configFile); - return ret; + return virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "nwfilter-edit", xml); } int -virNWFilterSaveConfig(const char *configDir, +virNWFilterSaveConfig(const char *configFile, virNWFilterDefPtr def) { int ret = -1; @@ -2799,7 +2790,7 @@ virNWFilterSaveConfig(const char *configDir, if (!(xml = virNWFilterDefFormat(def))) goto cleanup; - if (virNWFilterSaveXML(configDir, def, xml) < 0) + if (virNWFilterSaveXML(configFile, def, xml) < 0) goto cleanup; ret = 0; @@ -2925,26 +2916,17 @@ virNWFilterTriggerVMFilterRebuild(void) int -virNWFilterDeleteDef(const char *configDir, +virNWFilterDeleteDef(const char *configFile, virNWFilterDefPtr def) { - int ret = -1; - char *configFile = NULL; - - if (!(configFile = virFileBuildPath(configDir, def->name, ".xml"))) - goto error; - if (unlink(configFile) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot remove config for %s"), def->name); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(configFile); - return ret; + return 0; } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5cac260..5a3f008 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -570,7 +570,7 @@ int virNWFilterTriggerVMFilterRebuild(void); int -virNWFilterDeleteDef(const char *configDir, +virNWFilterDeleteDef(const char *configFile, virNWFilterDefPtr def); virNWFilterDefPtr @@ -581,12 +581,12 @@ char * virNWFilterDefFormat(const virNWFilterDef *def); int -virNWFilterSaveXML(const char *configDir, +virNWFilterSaveXML(const char *configFile, virNWFilterDefPtr def, const char *xml); int -virNWFilterSaveConfig(const char *configDir, +virNWFilterSaveConfig(const char *configFile, virNWFilterDefPtr def); virNWFilterDefPtr diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 7410047..ec28b05 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -512,7 +512,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, /* We generated a UUID, make it permanent by saving the config to disk */ if (!def->uuid_specified && - virNWFilterSaveConfig(configDir, def) < 0) + virNWFilterSaveConfig(configFile, def) < 0) goto error; if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 5e62023..97d6952 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -495,6 +495,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterObjPtr obj = NULL; virNWFilterDefPtr objdef; virNWFilterPtr ret = NULL; + char *configFile = NULL; if (!driver->privileged) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -512,12 +513,15 @@ nwfilterDefineXML(virConnectPtr conn, if (virNWFilterDefineXMLEnsureACL(conn, def) < 0) goto cleanup; + if (!(configFile = virFileBuildPath(driver->configDir, def->name, ".xml"))) + goto cleanup; + if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def))) goto cleanup; def = NULL; objdef = virNWFilterObjGetDef(obj); - if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) { + if (virNWFilterSaveConfig(configFile, objdef) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; } @@ -525,6 +529,7 @@ nwfilterDefineXML(virConnectPtr conn, ret = virGetNWFilter(conn, objdef->name, objdef->uuid); cleanup: + VIR_FREE(configFile); virNWFilterDefFree(def); if (obj) virNWFilterObjUnlock(obj); @@ -541,6 +546,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) { virNWFilterObjPtr obj; virNWFilterDefPtr def; + char *configFile = NULL; int ret = -1; nwfilterDriverLock(); @@ -561,7 +567,10 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; } - if (virNWFilterDeleteDef(driver->configDir, def) < 0) + if (!(configFile = virFileBuildPath(driver->configDir, def->name, ".xml"))) + goto cleanup; + + if (virNWFilterDeleteDef(configFile, def) < 0) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); @@ -569,6 +578,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) ret = 0; cleanup: + VIR_FREE(configFile); if (obj) virNWFilterObjUnlock(obj); -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:42PM -0400, John Ferlan wrote:
In preparation for it becoming part of the nwfilter object, move all the processing of the generation of the configFile name into the driver code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 36 +++++++++--------------------------- src/conf/nwfilter_conf.h | 6 +++--- src/conf/virnwfilterobj.c | 2 +- src/nwfilter/nwfilter_driver.c | 14 ++++++++++++-- 4 files changed, 25 insertions(+), 33 deletions(-)
Same review as for the similar patch from secret cleanup series :). Pavel
Only save the config when using a generated UUID if we were able to create an object for the def. There could have been "other reasons" for the assignment to fail, so saving the config could be incorrect. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index ec28b05..e37acf0 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -510,14 +510,14 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, goto error; } + if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) + goto error; + /* We generated a UUID, make it permanent by saving the config to disk */ if (!def->uuid_specified && virNWFilterSaveConfig(configFile, def) < 0) goto error; - if (!(obj = virNWFilterObjListAssignDef(nwfilters, def))) - goto error; - VIR_FREE(configFile); return obj; -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:43PM -0400, John Ferlan wrote:
Only save the config when using a generated UUID if we were able to create an object for the def. There could have been "other reasons" for the assignment to fail, so saving the config could be incorrect.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnwfilterobj.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK Pavel
Move into virnwfilterobj, rename the API to virNWFilterObjSaveConfig, and reorder the arguments. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 20 -------------------- src/conf/nwfilter_conf.h | 4 ---- src/conf/virnwfilterobj.c | 23 ++++++++++++++++++++++- src/conf/virnwfilterobj.h | 4 ++++ src/libvirt_private.syms | 3 ++- src/nwfilter/nwfilter_driver.c | 2 +- 6 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 6b67c3a..8925b2b 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2780,26 +2780,6 @@ virNWFilterSaveXML(const char *configFile, } -int -virNWFilterSaveConfig(const char *configFile, - virNWFilterDefPtr def) -{ - int ret = -1; - char *xml; - - if (!(xml = virNWFilterDefFormat(def))) - goto cleanup; - - if (virNWFilterSaveXML(configFile, def, xml) < 0) - goto cleanup; - - ret = 0; - cleanup: - VIR_FREE(xml); - return ret; -} - - int nCallbackDriver; #define MAX_CALLBACK_DRIVER 10 static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5a3f008..2287689 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -585,10 +585,6 @@ virNWFilterSaveXML(const char *configFile, virNWFilterDefPtr def, const char *xml); -int -virNWFilterSaveConfig(const char *configFile, - virNWFilterDefPtr def); - virNWFilterDefPtr virNWFilterDefParseString(const char *xml); diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index e37acf0..898ca7f 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -487,6 +487,27 @@ virNWFilterObjListExport(virConnectPtr conn, } +int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configFile) +{ + virNWFilterDefPtr def = obj->def; + int ret = -1; + char *xml; + + if (!(xml = virNWFilterDefFormat(def))) + goto cleanup; + + if (virNWFilterSaveXML(configFile, def, xml) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(xml); + return ret; +} + + static virNWFilterObjPtr virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, const char *configDir, @@ -515,7 +536,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, /* We generated a UUID, make it permanent by saving the config to disk */ if (!def->uuid_specified && - virNWFilterSaveConfig(configFile, def) < 0) + virNWFilterObjSaveConfig(obj, configFile) < 0) goto error; VIR_FREE(configFile); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 29d9086..f6a14de 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -98,6 +98,10 @@ virNWFilterObjListExport(virConnectPtr conn, virNWFilterObjListFilter aclfilter); int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configFile); + +int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 170ecce..4b334a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -740,7 +740,7 @@ virNWFilterRuleIsProtocolEthernet; virNWFilterRuleIsProtocolIPv4; virNWFilterRuleIsProtocolIPv6; virNWFilterRuleProtocolTypeToString; -virNWFilterSaveConfig; +virNWFilterSaveXML; virNWFilterTriggerVMFilterRebuild; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; @@ -975,6 +975,7 @@ virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; virNWFilterObjLock; +virNWFilterObjSaveConfig; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 97d6952..31e4867 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -521,7 +521,7 @@ nwfilterDefineXML(virConnectPtr conn, def = NULL; objdef = virNWFilterObjGetDef(obj); - if (virNWFilterSaveConfig(configFile, objdef) < 0) { + if (virNWFilterObjSaveConfig(obj, configFile) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; } -- 2.9.3
On Mon, Apr 24, 2017 at 03:18:44PM -0400, John Ferlan wrote:
Move into virnwfilterobj, rename the API to virNWFilterObjSaveConfig, and reorder the arguments.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 20 -------------------- src/conf/nwfilter_conf.h | 4 ---- src/conf/virnwfilterobj.c | 23 ++++++++++++++++++++++- src/conf/virnwfilterobj.h | 4 ++++ src/libvirt_private.syms | 3 ++- src/nwfilter/nwfilter_driver.c | 2 +- 6 files changed, 29 insertions(+), 27 deletions(-)
I don't see any benefit of moving the function, it operates only on virNWFilterDefPtr so the current place is suitable and passing the whole obj instead of def doesn't improve anything, unless there is some other agenda and some future patches will benefit from this change. Pavel
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 6b67c3a..8925b2b 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2780,26 +2780,6 @@ virNWFilterSaveXML(const char *configFile, }
-int -virNWFilterSaveConfig(const char *configFile, - virNWFilterDefPtr def) -{ - int ret = -1; - char *xml; - - if (!(xml = virNWFilterDefFormat(def))) - goto cleanup; - - if (virNWFilterSaveXML(configFile, def, xml) < 0) - goto cleanup; - - ret = 0; - cleanup: - VIR_FREE(xml); - return ret; -} - - int nCallbackDriver; #define MAX_CALLBACK_DRIVER 10 static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5a3f008..2287689 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -585,10 +585,6 @@ virNWFilterSaveXML(const char *configFile, virNWFilterDefPtr def, const char *xml);
-int -virNWFilterSaveConfig(const char *configFile, - virNWFilterDefPtr def); - virNWFilterDefPtr virNWFilterDefParseString(const char *xml);
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index e37acf0..898ca7f 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -487,6 +487,27 @@ virNWFilterObjListExport(virConnectPtr conn, }
+int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configFile) +{ + virNWFilterDefPtr def = obj->def; + int ret = -1; + char *xml; + + if (!(xml = virNWFilterDefFormat(def))) + goto cleanup; + + if (virNWFilterSaveXML(configFile, def, xml) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(xml); + return ret; +} + + static virNWFilterObjPtr virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, const char *configDir, @@ -515,7 +536,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters,
/* We generated a UUID, make it permanent by saving the config to disk */ if (!def->uuid_specified && - virNWFilterSaveConfig(configFile, def) < 0) + virNWFilterObjSaveConfig(obj, configFile) < 0) goto error;
VIR_FREE(configFile); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 29d9086..f6a14de 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -98,6 +98,10 @@ virNWFilterObjListExport(virConnectPtr conn, virNWFilterObjListFilter aclfilter);
int +virNWFilterObjSaveConfig(virNWFilterObjPtr obj, + const char *configFile); + +int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 170ecce..4b334a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -740,7 +740,7 @@ virNWFilterRuleIsProtocolEthernet; virNWFilterRuleIsProtocolIPv4; virNWFilterRuleIsProtocolIPv6; virNWFilterRuleProtocolTypeToString; -virNWFilterSaveConfig; +virNWFilterSaveXML; virNWFilterTriggerVMFilterRebuild; virNWFilterUnlockFilterUpdates; virNWFilterUnRegisterCallbackDriver; @@ -975,6 +975,7 @@ virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; virNWFilterObjLock; +virNWFilterObjSaveConfig; virNWFilterObjTestUnassignDef; virNWFilterObjUnlock; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 97d6952..31e4867 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -521,7 +521,7 @@ nwfilterDefineXML(virConnectPtr conn, def = NULL; objdef = virNWFilterObjGetDef(obj);
- if (virNWFilterSaveConfig(configFile, objdef) < 0) { + if (virNWFilterObjSaveConfig(obj, configFile) < 0) { virNWFilterObjListRemove(driver->nwfilters, obj); goto cleanup; } -- 2.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/25/2017 10:37 AM, Pavel Hrdina wrote:
On Mon, Apr 24, 2017 at 03:18:44PM -0400, John Ferlan wrote:
Move into virnwfilterobj, rename the API to virNWFilterObjSaveConfig, and reorder the arguments.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 20 -------------------- src/conf/nwfilter_conf.h | 4 ---- src/conf/virnwfilterobj.c | 23 ++++++++++++++++++++++- src/conf/virnwfilterobj.h | 4 ++++ src/libvirt_private.syms | 3 ++- src/nwfilter/nwfilter_driver.c | 2 +- 6 files changed, 29 insertions(+), 27 deletions(-)
I don't see any benefit of moving the function, it operates only on virNWFilterDefPtr so the current place is suitable and passing the whole obj instead of def doesn't improve anything, unless there is some other agenda and some future patches will benefit from this change.
Pavel
Well... imagine if you will that... nwfilterDefineXML calls virNWFilterObjListAssignDef which at some point in the future would consume "configFile" as well as def. IOW: it becomes part of the object. Again, I'm working backwards here in order to make the future easier. Anyway, I'll rethink/rework that logic later so patches 13 and 15 end up being dropped. Tks - John
participants (2)
-
John Ferlan -
Pavel Hrdina