On 07/13/2017 10:40 AM, Michal Privoznik wrote:
> 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(-)
>>
FWIW: I've pushed 1, 4, and 8-13 since they were ACK. Since 2, 3, and
5-7 are being dropped and I want to insert a revert prior to here
(separate patch posted) - I'll respond to the queries from 14-17, but
they will get reposted as a v2 once the revert is in.
>> 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.
>
True, but in order to "show" the progression from point A to point B
*and* because of that recursive locking algorithm currently in place, I
essentially lifted code from virobject.c (this is from virObjectNew and
virObjectLockableNew).
>>
>> 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.
>
Oh right.... Good eyes! My testing wouldn't see it because I tested
the whole series which would essentially replace this.
>> + 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)?
>
Sure that's fine, same result.
>> }
>>
>> @@ -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?
>
Let's see, looks like commit id '46a811db07' added the if we cannot find
by UUID, then let's find by Name and if the name exists, then cause
failure because we have a defined Name with a different UUID.
But it didn't adjust this algorithm; however, rather than change to
passing true, since this is the only caller, the @cmpUUIDs should be
removed from virNWFilterDefEqual. In a separate followup patch of course.
As noted above - I'll fix stuff up here, add the patch, and post a v2
Tks -
John
>> @@ -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
