[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 14/17] nwfilter: Add @refs logic to __virNWFilterObj




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 redhat com>
>> ---
>>  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
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]