On 06/02/2017 12:25 PM, John Ferlan wrote:
> "Simple" conversion to the virObjectLockable object isn't quite possible
> yet because the nwfilter lock requires usage of recursive locking due to
> algorithms handing filter "<rule>"'s and "<filterref>"'s. The list search
> logic would also benefit from using hash tables for lookups. So this patch
> is step 1 in the process - add the @refs to _virNWFilterObj and modify the
> algorithms to use (a temporary) virNWFilterObj{Ref|Unref} in order to set
> things up for the list logic to utilize virObjectLockable hash tables.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/conf/virnwfilterobj.c | 75 ++++++++++++++++++++++++++--------
> src/conf/virnwfilterobj.h | 15 ++++---
> src/libvirt_private.syms | 5 ++-
> src/nwfilter/nwfilter_driver.c | 13 +++---
> src/nwfilter/nwfilter_gentech_driver.c | 11 +++--
> 5 files changed, 80 insertions(+), 39 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index b21b570..99be59c 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -23,6 +23,7 @@
> #include "datatypes.h"
>
> #include "viralloc.h"
> +#include "viratomic.h"
> #include "virerror.h"
> #include "virfile.h"
> #include "virlog.h"
> @@ -33,8 +34,15 @@
>
> VIR_LOG_INIT("conf.virnwfilterobj");
>
> +static void
> +virNWFilterObjLock(virNWFilterObjPtr obj);
> +
> +static void
> +virNWFilterObjUnlock(virNWFilterObjPtr obj);
> +
> struct _virNWFilterObj {
> virMutex lock;
> + int refs;
>
> bool wantRemoved;
>
> @@ -67,11 +75,24 @@ virNWFilterObjNew(virNWFilterDefPtr def)
>
> virNWFilterObjLock(obj);
> obj->def = def;
> + virAtomicIntSet(&obj->refs, 1);
Technically, this doesn't need to be virAtomic. Bare assignment would
work as: a) the object is locked at this point, b) there's no other
reference to the object (we are just creating the first one). But I'm
fine with leaving this as is, just wanted to point out my comment.
>
> return obj;
> }
>
>
> +void
> +virNWFilterObjEndAPI(virNWFilterObjPtr *obj)
> +{
> + if (!*obj)
> + return;
> +
> + virNWFilterObjUnlock(*obj);
> + virNWFilterObjUnref(*obj);
> + *obj = NULL;
> +}
> +
> +
> virNWFilterDefPtr
> virNWFilterObjGetDef(virNWFilterObjPtr obj)
> {
> @@ -109,12 +130,33 @@ virNWFilterObjFree(virNWFilterObjPtr obj)
> }
>
>
> +virNWFilterObjPtr
> +virNWFilterObjRef(virNWFilterObjPtr obj)
> +{
> + if (obj)
> + virAtomicIntInc(&obj->refs);
> + return obj;
> +}
> +
> +
> +bool
> +virNWFilterObjUnref(virNWFilterObjPtr obj)
> +{
> + bool lastRef;
> + if (obj)
> + return false;
This can hardly work. The condition needs to be inverted.
> + if ((lastRef = virAtomicIntDecAndTest(&obj->refs)))
> + virNWFilterObjFree(obj);
> + return !lastRef;
> +}
> +
> +
> void
> virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
> {
> size_t i;
> for (i = 0; i < nwfilters->count; i++)
> - virNWFilterObjFree(nwfilters->objs[i]);
> + virNWFilterObjUnref(nwfilters->objs[i]);
> VIR_FREE(nwfilters->objs);
> VIR_FREE(nwfilters);
> }
> @@ -143,7 +185,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
> virNWFilterObjLock(nwfilters->objs[i]);
> if (nwfilters->objs[i] == obj) {
> virNWFilterObjUnlock(nwfilters->objs[i]);
> - virNWFilterObjFree(nwfilters->objs[i]);
> + virNWFilterObjUnref(nwfilters->objs[i]);
>
> VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
> break;
> @@ -166,7 +208,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
> virNWFilterObjLock(obj);
> def = obj->def;
> if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
> - return obj;
> + return virNWFilterObjRef(obj);
> virNWFilterObjUnlock(obj);
> }
>
> @@ -187,7 +229,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
> virNWFilterObjLock(obj);
> def = obj->def;
> if (STREQ_NULLABLE(def->name, name))
> - return obj;
> + return virNWFilterObjRef(obj);
> virNWFilterObjUnlock(obj);
> }
>
> @@ -210,7 +252,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
> if (virNWFilterObjWantRemoved(obj)) {
> virReportError(VIR_ERR_NO_NWFILTER,
> _("Filter '%s' is in use."), filtername);
> - virNWFilterObjUnlock(obj);
> + virNWFilterObjEndAPI(&obj);
> return NULL;
Can we remove this "return NULL" line and rely on "return obj" which
follow immediately (not to be seen in the context here though)?
> }
>
> @@ -245,7 +287,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
> if (obj) {
> rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def,
> filtername);
> - virNWFilterObjUnlock(obj);
> + virNWFilterObjEndAPI(&obj);
> if (rc < 0)
> break;
> }
> @@ -338,10 +380,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> _("filter with same UUID but different name "
> "('%s') already exists"),
> objdef->name);
> - virNWFilterObjUnlock(obj);
> + virNWFilterObjEndAPI(&obj);
> return NULL;
> }
> - virNWFilterObjUnlock(obj);
> + virNWFilterObjEndAPI(&obj);
> } else {
> if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> @@ -351,7 +393,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> virReportError(VIR_ERR_OPERATION_FAILED,
> _("filter '%s' already exists with uuid %s"),
> def->name, uuidstr);
> - virNWFilterObjUnlock(obj);
> + virNWFilterObjEndAPI(&obj);
> return NULL;
> }
> }
> @@ -362,7 +404,6 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> return NULL;
> }
>
> -
> if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>
> objdef = obj->def;
I've got an unrelated question: here, after this line you can find the
following code:
if (virNWFilterDefEqual(def, objdef, false)) {
virNWFilterDefFree(objdef);
obj->def = def;
return obj;
}
Firstly, I think we can s/false/true/ because if UUIDs were not the
same, we would have errored out way sooner. But more importantly, if
definition is equal what's the point in replacing it?
> @@ -376,7 +417,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> /* trigger the update on VMs referencing the filter */
> if (virNWFilterTriggerVMFilterRebuild() < 0) {
> obj->newDef = NULL;
> - virNWFilterObjUnlock(obj);
> + virNWFilterObjEndAPI(&obj);
> return NULL;
> }
>
> @@ -397,12 +438,12 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0)
> goto error;
>
> - return obj;
> + return virNWFilterObjRef(obj);
>
> error:
> obj->def = NULL;
> virNWFilterObjUnlock(obj);
> - virNWFilterObjFree(obj);
> + virNWFilterObjUnref(obj);
> return NULL;
> }
>
> @@ -600,8 +641,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
> continue;
>
> obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name);
> - if (obj)
> - virNWFilterObjUnlock(obj);
> +
> + virNWFilterObjEndAPI(&obj);
> }
>
> VIR_DIR_CLOSE(dir);
> @@ -609,14 +650,14 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
> }
>
>
> -void
> +static void
> virNWFilterObjLock(virNWFilterObjPtr obj)
> {
> virMutexLock(&obj->lock);
> }
>
>
> -void
> +static void
> virNWFilterObjUnlock(virNWFilterObjPtr obj)
> {
> virMutexUnlock(&obj->lock);
> diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
> index 85c8ead..31aa345 100644
> --- a/src/conf/virnwfilterobj.h
> +++ b/src/conf/virnwfilterobj.h
> @@ -41,6 +41,9 @@ struct _virNWFilterDriverState {
> bool watchingFirewallD;
> };
>
> +void
> +virNWFilterObjEndAPI(virNWFilterObjPtr *obj);
> +
> virNWFilterDefPtr
> virNWFilterObjGetDef(virNWFilterObjPtr obj);
>
> @@ -50,6 +53,12 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj);
> bool
> virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
>
> +virNWFilterObjPtr
> +virNWFilterObjRef(virNWFilterObjPtr obj);
> +
> +bool
> +virNWFilterObjUnref(virNWFilterObjPtr obj);
> +
ACK
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
