[libvirt] [PATCH 0/6] Alternative way of turning NWFilterObj(list) into virObject
This is heavily based on John's work (except 1/6). The difference to his patches is in using virObjectRecursiveLockableNew() for recursive locks instead of RW locks and lock promoting. Also, in 6/6 I was more confident and removed driver lock from define/undefine APIs. John Ferlan (1): nwfilter: Remove need for nwfilterDriverLock in some API's Michal Privoznik (5): virObject: use virReportSystemError if applicable virObject: Introduce virObjectRecursiveLockableNew virNWFilterObj: Turn into virObjectLockable conf: Introduce and use virNWFilterObjEndAPI nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable cfg.mk | 1 - src/conf/virdomainobjlist.c | 3 +- src/conf/virnwfilterobj.c | 504 +++++++++++++++++++++------------ src/conf/virnwfilterobj.h | 12 +- src/libvirt_private.syms | 5 +- src/nwfilter/nwfilter_driver.c | 65 ++--- src/nwfilter/nwfilter_gentech_driver.c | 11 +- src/util/virobject.c | 30 +- src/util/virobject.h | 4 + 9 files changed, 382 insertions(+), 253 deletions(-) -- 2.13.6
When initializing a mutex (either regular or RW) the virMutexInit() and virRWLockInit() functions set errno and return -1. It's a pity we don't use virReportSystemError() in that case rather plain virReportError(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virobject.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 1723df6b2..b2fc63aec 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -273,8 +273,8 @@ virObjectLockableNew(virClassPtr klass) return NULL; if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to initialize mutex")); + virReportSystemError(errno, "%s", + _("Unable to initialize mutex")); virObjectUnref(obj); return NULL; } @@ -299,8 +299,8 @@ virObjectRWLockableNew(virClassPtr klass) return NULL; if (virRWLockInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to initialize RW lock")); + virReportSystemError(errno, "%s", + _("Unable to initialize RW lock")); virObjectUnref(obj); return NULL; } -- 2.13.6
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks. Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,6 +2417,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectRecursiveLockableNew; virObjectRef; virObjectRWLockableNew; virObjectRWLockRead; diff --git a/src/util/virobject.c b/src/util/virobject.c index b2fc63aec..1d82e826b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -257,8 +257,9 @@ virObjectNew(virClassPtr klass) } -void * -virObjectLockableNew(virClassPtr klass) +static void * +virObjectLockableNewInternal(virClassPtr klass, + bool recursive) { virObjectLockablePtr obj; @@ -272,7 +273,8 @@ virObjectLockableNew(virClassPtr klass) if (!(obj = virObjectNew(klass))) return NULL; - if (virMutexInit(&obj->lock) < 0) { + if ((!recursive && virMutexInit(&obj->lock) < 0) || + (recursive && virMutexInitRecursive(&obj->lock) < 0)) { virReportSystemError(errno, "%s", _("Unable to initialize mutex")); virObjectUnref(obj); @@ -283,6 +285,20 @@ virObjectLockableNew(virClassPtr klass) } +void * +virObjectLockableNew(virClassPtr klass) +{ + return virObjectLockableNewInternal(klass, false); +} + + +void * +virObjectRecursiveLockableNew(virClassPtr klass) +{ + return virObjectLockableNewInternal(klass, true); +} + + void * virObjectRWLockableNew(virClassPtr klass) { diff --git a/src/util/virobject.h b/src/util/virobject.h index ac6cf22f9..367d505ae 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -116,6 +116,10 @@ void * virObjectLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +void * +virObjectRecursiveLockableNew(virClassPtr klass) + ATTRIBUTE_NONNULL(1); + void * virObjectRWLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); -- 2.13.6
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks.
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,6 +2417,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectRecursiveLockableNew;
I think this was NACK'd last time since we did not want to promote usage of recursive locks in the code. If we provide an object that provides recursive locking we de-facto promote this usage. As Pavel stated in his review. Ideally the NWfilter code will be converted to a less convoluted locking not requiring recursive locks prior to this so that we don't have to add recursive locking at all.
On 02/12/2018 01:10 PM, Peter Krempa wrote:
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks.
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,6 +2417,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectRecursiveLockableNew;
I think this was NACK'd last time since we did not want to promote usage of recursive locks in the code. If we provide an object that provides recursive locking we de-facto promote this usage.
As Pavel stated in his review. Ideally the NWfilter code will be converted to a less convoluted locking not requiring recursive locks prior to this so that we don't have to add recursive locking at all.
I think that is far from happening. And I don't see any difference between virObjectRecursiveLockableNew() and virMutexInitRecursive() in terms of promoting something. Can you shed more light where do you see the difference? Michal
On Mon, Feb 12, 2018 at 01:16:28PM +0100, Michal Privoznik wrote:
On 02/12/2018 01:10 PM, Peter Krempa wrote:
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks.
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,6 +2417,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectRecursiveLockableNew;
I think this was NACK'd last time since we did not want to promote usage of recursive locks in the code. If we provide an object that provides recursive locking we de-facto promote this usage.
As Pavel stated in his review. Ideally the NWfilter code will be converted to a less convoluted locking not requiring recursive locks prior to this so that we don't have to add recursive locking at all.
I think that is far from happening. And I don't see any difference between virObjectRecursiveLockableNew() and virMutexInitRecursive() in terms of promoting something. Can you shed more light where do you see the difference?
IMHO the difference is what Peter already wrote, creating new object that automatically uses recursive locks makes it easier to use and somehow promotes it, because nowadays we are rewriting a lot of code to use objects. The other argument is that we should avoid recursive locks which includes not introducing new code that works with recursive locks. I know that NWFilter code is complex and removing recursive locks is not an easy task, but for the long run I think it's worth it, it will make the code cleaner and easier to follow. In conclusion, I still stand behind my NACK. Pavel
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/16/2018 09:34 AM, Pavel Hrdina wrote:
On Mon, Feb 12, 2018 at 01:16:28PM +0100, Michal Privoznik wrote:
On 02/12/2018 01:10 PM, Peter Krempa wrote:
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks.
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,6 +2417,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectRecursiveLockableNew;
I think this was NACK'd last time since we did not want to promote usage of recursive locks in the code. If we provide an object that provides recursive locking we de-facto promote this usage.
As Pavel stated in his review. Ideally the NWfilter code will be converted to a less convoluted locking not requiring recursive locks prior to this so that we don't have to add recursive locking at all.
I think that is far from happening. And I don't see any difference between virObjectRecursiveLockableNew() and virMutexInitRecursive() in terms of promoting something. Can you shed more light where do you see the difference?
IMHO the difference is what Peter already wrote, creating new object that automatically uses recursive locks makes it easier to use and somehow promotes it, because nowadays we are rewriting a lot of code to use objects.
The other argument is that we should avoid recursive locks which includes not introducing new code that works with recursive locks.
This can be viewed as rewrite of existing code, not completely new code.
I know that NWFilter code is complex and removing recursive locks is not an easy task, but for the long run I think it's worth it, it will make the code cleaner and easier to follow.
Right, that the ideal goal. But as I said it's far from happening. I think it was you who when trying to fix some issue in NWFilter drew call graph in NWFilter driver and realized how complicated it is. That's why I don't see it happening anywhere in near future. Also, if we really have multiple entry points as Dan mentioned earlier can we really fix this? I mean there are multiple locks that need to be acquired when touching a virNWFilterObj. The advantage of reentrant mutex is that we will not get a dead lock scenario if two functions fight over lock. Anyway, it's a pity that we are stuck on this patch while reworking the vir*ObjList code.
In conclusion, I still stand behind my NACK.
Okay, in that case I'm gonna ACK John's patches. It's very unfortunate that because of you NACKing this patch we will have to have lock promoting. Michal
On Fri, Feb 16, 2018 at 09:52:53 +0100, Michal Privoznik wrote:
On 02/16/2018 09:34 AM, Pavel Hrdina wrote:
On Mon, Feb 12, 2018 at 01:16:28PM +0100, Michal Privoznik wrote:
On 02/12/2018 01:10 PM, Peter Krempa wrote:
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks.
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms
[...]
This can be viewed as rewrite of existing code, not completely new code.
I know that NWFilter code is complex and removing recursive locks is not an easy task, but for the long run I think it's worth it, it will make the code cleaner and easier to follow.
Right, that the ideal goal. But as I said it's far from happening. I think it was you who when trying to fix some issue in NWFilter drew call graph in NWFilter driver and realized how complicated it is. That's why I don't see it happening anywhere in near future. Also, if we really have multiple entry points as Dan mentioned earlier can we really fix this? I mean there are multiple locks that need to be acquired when touching a virNWFilterObj. The advantage of reentrant mutex is that we will not get a dead lock scenario if two functions fight over lock.
Anyway, it's a pity that we are stuck on this patch while reworking the vir*ObjList code.
So and why can't we keep the NWfilter code as-is until the locking is sanitized first? It is working so I don't see a reason to try to rewrite it to objects if it is not trivially possible.
On 02/16/2018 10:08 AM, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 09:52:53 +0100, Michal Privoznik wrote:
On 02/16/2018 09:34 AM, Pavel Hrdina wrote:
On Mon, Feb 12, 2018 at 01:16:28PM +0100, Michal Privoznik wrote:
On 02/12/2018 01:10 PM, Peter Krempa wrote:
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote:
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks.
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms
[...]
This can be viewed as rewrite of existing code, not completely new code.
I know that NWFilter code is complex and removing recursive locks is not an easy task, but for the long run I think it's worth it, it will make the code cleaner and easier to follow.
Right, that the ideal goal. But as I said it's far from happening. I think it was you who when trying to fix some issue in NWFilter drew call graph in NWFilter driver and realized how complicated it is. That's why I don't see it happening anywhere in near future. Also, if we really have multiple entry points as Dan mentioned earlier can we really fix this? I mean there are multiple locks that need to be acquired when touching a virNWFilterObj. The advantage of reentrant mutex is that we will not get a dead lock scenario if two functions fight over lock.
Anyway, it's a pity that we are stuck on this patch while reworking the vir*ObjList code.
So and why can't we keep the NWfilter code as-is until the locking is sanitized first? It is working so I don't see a reason to try to rewrite it to objects if it is not trivially possible.
Well, I find it somewhat disappointing. The patches that John and I proposed make things better. But because they don't make it 100% better they are NACKed. But I can live with having two different implementations for vir*ObjList if that's what we want. Or if it's better than having either John's or mines patches merged. I think otherwise though. Michal
On 02/16/2018 04:47 AM, Michal Privoznik wrote:
On 02/16/2018 10:08 AM, Peter Krempa wrote:
On Fri, Feb 16, 2018 at 09:52:53 +0100, Michal Privoznik wrote:
On 02/16/2018 09:34 AM, Pavel Hrdina wrote:
On Mon, Feb 12, 2018 at 01:16:28PM +0100, Michal Privoznik wrote:
On 02/12/2018 01:10 PM, Peter Krempa wrote:
On Mon, Feb 12, 2018 at 11:52:49 +0100, Michal Privoznik wrote: > Sometimes we need the lock in virObjectLockable to be recursive. > Because of the nature of pthreads we don't need a special class > for that - the pthread_* APIs don't distinguish between normal > and recursive locks. > > Based-on-work-of: John Ferlan <jferlan@redhat.com> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > src/libvirt_private.syms | 1 + > src/util/virobject.c | 22 +++++++++++++++++++--- > src/util/virobject.h | 4 ++++ > 3 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 3b14d7d15..fcf378105 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms
[...]
This can be viewed as rewrite of existing code, not completely new code.
I know that NWFilter code is complex and removing recursive locks is not an easy task, but for the long run I think it's worth it, it will make the code cleaner and easier to follow.
Right, that the ideal goal. But as I said it's far from happening. I think it was you who when trying to fix some issue in NWFilter drew call graph in NWFilter driver and realized how complicated it is. That's why I don't see it happening anywhere in near future. Also, if we really have multiple entry points as Dan mentioned earlier can we really fix this? I mean there are multiple locks that need to be acquired when touching a virNWFilterObj. The advantage of reentrant mutex is that we will not get a dead lock scenario if two functions fight over lock.
Anyway, it's a pity that we are stuck on this patch while reworking the vir*ObjList code.
So and why can't we keep the NWfilter code as-is until the locking is sanitized first? It is working so I don't see a reason to try to rewrite it to objects if it is not trivially possible.
That assumes some day locks will be sanitized or the nwfilter code will be rewritten. The chances of that happening are slim and nil AFAICT. Perhaps the "best" one could do is keep a cache of already locked objects to use for the lookup code rather than just using the "normal" find object in list logic. That way the code doesn't need to have recursive locks. Given that change in the code is feared - what chance does making any change at being successful? At least the changes we're discussing now allow the code to use existing mechanisms for object list and object mgmt. Keeping the nwfilter code as-is is an option, but to say it's working would seem to gloss over the fact that it's using recursive locks that can be used for write, but aren't "yet" unless someone removes the nwfilter driver, filter update, and callback locks in the define, undefine, and reload paths that keep any chance of an update in a single threaded path.
Well, I find it somewhat disappointing. The patches that John and I proposed make things better. But because they don't make it 100% better they are NACKed. But I can live with having two different implementations for vir*ObjList if that's what we want. Or if it's better than having either John's or mines patches merged. I think otherwise though.
At some point in time using virObjectLockable for managing refs, locks, and memory reclamation was deemed making things better. At some point in libvirt history virMutexInitRecursive was deemed to be OK, but now it's not for the purpose of using virObject* code for nwfilters. Eventually RWReadLocks came along which by nature allow the same thread to hold multiple concurrent read locks as long as the same number of matching unlocks is done. So it seemed reasonable to me to have virNWFilterObj use an RW lock, but in doing so the problem is we "know" it's a read lock, so in order to update the object getting a write lock is the "normal" thing to do. That meant promotion because it's undefined what happens if you have a read lock and you get a write lock. So maybe we "use" that knowledge that the define, undefine, and reload paths are single threaded and either avoid the promotion altogether or heavily document the NWFilterObj promotion to indicate the issue behind doing this promotion without the additional protection so that no one copies the logic. Or we do nothing because things "work". John
On 02/12/2018 05:52 AM, Michal Privoznik wrote:
Sometimes we need the lock in virObjectLockable to be recursive. Because of the nature of pthreads we don't need a special class for that - the pthread_* APIs don't distinguish between normal and recursive locks.
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 22 +++++++++++++++++++--- src/util/virobject.h | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-)
Adding recursive locks to virObject* was already rejected once before: Patch: https://www.redhat.com/archives/libvir-list/2017-May/msg01183.html Responses: https://www.redhat.com/archives/libvir-list/2017-May/msg01212.html https://www.redhat.com/archives/libvir-list/2017-May/msg01215.html When you added RWLock's in commit '77f4593b' (afterwards) - I began thinking - hey the NWFilter code could use those. So I essentially ditched the effort that used trylock: https://www.redhat.com/archives/libvir-list/2017-July/msg00673.html in favor of one that used RWLocks: https://www.redhat.com/archives/libvir-list/2017-October/msg00264.html Unlike previous alterations to driver/vir*objlist code - I found that I needed to change the vir*Obj before changing the vir*ObjList, although I have forgotten why at this point. Eventually I also discovered why avocado test was causing libvirtd to go defunct (as noted in the cover of the v3). That was due to the way the test restarts libvirtd prior to each run and an unrelated issue in nodedev w/r/t initialization, see: https://www.redhat.com/archives/libvir-list/2017-December/msg00312.html John
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..fcf378105 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,6 +2417,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectRecursiveLockableNew; virObjectRef; virObjectRWLockableNew; virObjectRWLockRead; diff --git a/src/util/virobject.c b/src/util/virobject.c index b2fc63aec..1d82e826b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -257,8 +257,9 @@ virObjectNew(virClassPtr klass) }
-void * -virObjectLockableNew(virClassPtr klass) +static void * +virObjectLockableNewInternal(virClassPtr klass, + bool recursive) { virObjectLockablePtr obj;
@@ -272,7 +273,8 @@ virObjectLockableNew(virClassPtr klass) if (!(obj = virObjectNew(klass))) return NULL;
- if (virMutexInit(&obj->lock) < 0) { + if ((!recursive && virMutexInit(&obj->lock) < 0) || + (recursive && virMutexInitRecursive(&obj->lock) < 0)) { virReportSystemError(errno, "%s", _("Unable to initialize mutex")); virObjectUnref(obj); @@ -283,6 +285,20 @@ virObjectLockableNew(virClassPtr klass) }
+void * +virObjectLockableNew(virClassPtr klass) +{ + return virObjectLockableNewInternal(klass, false); +} + + +void * +virObjectRecursiveLockableNew(virClassPtr klass) +{ + return virObjectLockableNewInternal(klass, true); +} + + void * virObjectRWLockableNew(virClassPtr klass) { diff --git a/src/util/virobject.h b/src/util/virobject.h index ac6cf22f9..367d505ae 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -116,6 +116,10 @@ void * virObjectLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1);
+void * +virObjectRecursiveLockableNew(virClassPtr klass) + ATTRIBUTE_NONNULL(1); + void * virObjectRWLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1);
The virNWFilterObj requires recursive locks, otherwise it is regular virObject. So when creating the object we must call virObjectRecursiveLockableNew(). Other than that, this is pure replacement of virNWFilterObj*() APIs with thei virObject*() counterparts. Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virnwfilterobj.c | 121 ++++++++++++++++----------------- src/conf/virnwfilterobj.h | 6 -- src/libvirt_private.syms | 2 - src/nwfilter/nwfilter_driver.c | 10 +-- src/nwfilter/nwfilter_gentech_driver.c | 10 +-- 5 files changed, 67 insertions(+), 82 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 87d7e7270..a472ff531 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -34,7 +34,7 @@ VIR_LOG_INIT("conf.virnwfilterobj"); struct _virNWFilterObj { - virMutex lock; + virObjectLockable parent; bool wantRemoved; @@ -42,32 +42,54 @@ struct _virNWFilterObj { virNWFilterDefPtr newDef; }; +static virClassPtr virNWFilterObjClass; +static void virNWFilterObjDispose(void *obj); + struct _virNWFilterObjList { size_t count; virNWFilterObjPtr *objs; }; +static int virNWFilterObjOnceInit(void) +{ + if (!(virNWFilterObjClass = virClassNew(virClassForObjectLockable(), + "virNWFilterObj", + sizeof(virNWFilterObj), + virNWFilterObjDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterObj) + static virNWFilterObjPtr virNWFilterObjNew(void) { virNWFilterObjPtr obj; - if (VIR_ALLOC(obj) < 0) + if (virNWFilterObjInitialize() < 0) return NULL; - if (virMutexInitRecursive(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virObjectRecursiveLockableNew(virNWFilterObjClass))) return NULL; - } - virNWFilterObjLock(obj); + virObjectLock(obj); return obj; } +static void +virNWFilterObjDispose(void *opaque) +{ + virNWFilterObjPtr obj = opaque; + + virNWFilterDefFree(obj->def); + virNWFilterDefFree(obj->newDef); +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -89,27 +111,12 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj) } -static void -virNWFilterObjFree(virNWFilterObjPtr obj) -{ - if (!obj) - return; - - virNWFilterDefFree(obj->def); - virNWFilterDefFree(obj->newDef); - - virMutexDestroy(&obj->lock); - - VIR_FREE(obj); -} - - void virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) { size_t i; for (i = 0; i < nwfilters->count; i++) - virNWFilterObjFree(nwfilters->objs[i]); + virObjectUnref(nwfilters->objs[i]); VIR_FREE(nwfilters->objs); VIR_FREE(nwfilters); } @@ -132,18 +139,18 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, { size_t i; - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjLock(nwfilters->objs[i]); + virObjectLock(nwfilters->objs[i]); if (nwfilters->objs[i] == obj) { - virNWFilterObjUnlock(nwfilters->objs[i]); - virNWFilterObjFree(nwfilters->objs[i]); + virObjectUnlock(nwfilters->objs[i]); + virObjectUnref(nwfilters->objs[i]); VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; } - virNWFilterObjUnlock(nwfilters->objs[i]); + virObjectUnlock(nwfilters->objs[i]); } } @@ -158,11 +165,11 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, for (i = 0; i < nwfilters->count; i++) { obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectLock(obj); def = obj->def; if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) return obj; - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); } return NULL; @@ -179,11 +186,11 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, for (i = 0; i < nwfilters->count; i++) { obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectLock(obj); def = obj->def; if (STREQ_NULLABLE(def->name, name)) return obj; - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); } return NULL; @@ -205,7 +212,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return NULL; } @@ -240,7 +247,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, if (obj) { rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); if (rc < 0) break; } @@ -322,10 +329,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, _("filter with same UUID but different name " "('%s') already exists"), objdef->name); - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return NULL; } - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -335,7 +342,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return NULL; } } @@ -360,7 +367,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return NULL; } @@ -375,8 +382,8 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) { - virNWFilterObjUnlock(obj); - virNWFilterObjFree(obj); + virObjectUnlock(obj); + virObjectUnref(obj); return NULL; } obj->def = def; @@ -395,10 +402,10 @@ virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, for (i = 0; i < nwfilters->count; i++) { virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectLock(obj); if (!filter || filter(conn, obj->def)) nfilters++; - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); } return nfilters; @@ -418,16 +425,16 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { virNWFilterObjPtr obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectLock(obj); def = obj->def; if (!filter || filter(conn, def)) { if (VIR_STRDUP(names[nnames], def->name) < 0) { - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); } return nnames; @@ -464,16 +471,16 @@ virNWFilterObjListExport(virConnectPtr conn, for (i = 0; i < nwfilters->count; i++) { obj = nwfilters->objs[i]; - virNWFilterObjLock(obj); + virObjectLock(obj); def = obj->def; if (!filter || filter(conn, def)) { if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) { - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); goto cleanup; } tmp_filters[nfilters++] = nwfilter; } - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); } *filters = tmp_filters; @@ -552,23 +559,9 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name); if (obj) - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); } VIR_DIR_CLOSE(dir); return ret; } - - -void -virNWFilterObjLock(virNWFilterObjPtr obj) -{ - virMutexLock(&obj->lock); -} - - -void -virNWFilterObjUnlock(virNWFilterObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 8e79518ed..9faba264a 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -105,10 +105,4 @@ int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir); -void -virNWFilterObjLock(virNWFilterObjPtr obj); - -void -virNWFilterObjUnlock(virNWFilterObjPtr obj); - #endif /* VIRNWFILTEROBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fcf378105..67e3ade0f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1052,9 +1052,7 @@ virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; virNWFilterObjListNumOfNWFilters; virNWFilterObjListRemove; -virNWFilterObjLock; virNWFilterObjTestUnassignDef; -virNWFilterObjUnlock; virNWFilterObjWantRemoved; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 717bce269..ffd603d70 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -466,7 +466,7 @@ nwfilterLookupByUUID(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return nwfilter; } @@ -496,7 +496,7 @@ nwfilterLookupByName(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return nwfilter; } @@ -591,7 +591,7 @@ nwfilterDefineXML(virConnectPtr conn, cleanup: virNWFilterDefFree(def); if (obj) - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -634,7 +634,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter) cleanup: if (obj) - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -667,7 +667,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, ret = virNWFilterDefFormat(def); cleanup: - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 840d419bb..106364c67 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -316,7 +316,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virNWFilterObjUnlock(inst->filters[i]); + virObjectUnlock(inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -427,7 +427,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); if (obj) - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -541,7 +541,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, vars))) { - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); return -1; } @@ -565,7 +565,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); if (rc < 0) return -1; } @@ -839,7 +839,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virNWFilterObjUnlock(obj); + virObjectUnlock(obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); -- 2.13.6
Have every API that is getting a virNWFilterObj from virNWFilterObjList grab a reference and at the same time wrap Unlock() + Unref() calls into virNWFilterObjEndAPI(). Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virnwfilterobj.c | 38 +++++++++++++++++++++------------- src/conf/virnwfilterobj.h | 3 +++ src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_driver.c | 14 +++++-------- src/nwfilter/nwfilter_gentech_driver.c | 11 +++++----- 5 files changed, 38 insertions(+), 29 deletions(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index a472ff531..6a54628b6 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -90,6 +90,18 @@ virNWFilterObjDispose(void *opaque) } +void +virNWFilterObjEndAPI(virNWFilterObjPtr *obj) +{ + if (!*obj) + return; + + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; +} + + virNWFilterDefPtr virNWFilterObjGetDef(virNWFilterObjPtr obj) { @@ -144,8 +156,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, for (i = 0; i < nwfilters->count; i++) { virObjectLock(nwfilters->objs[i]); if (nwfilters->objs[i] == obj) { - virObjectUnlock(nwfilters->objs[i]); - virObjectUnref(nwfilters->objs[i]); + virNWFilterObjEndAPI(&nwfilters->objs[i]); VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; @@ -168,7 +179,7 @@ virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, virObjectLock(obj); def = obj->def; if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return obj; + return virObjectRef(obj); virObjectUnlock(obj); } @@ -189,7 +200,7 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, virObjectLock(obj); def = obj->def; if (STREQ_NULLABLE(def->name, name)) - return obj; + return virObjectRef(obj); virObjectUnlock(obj); } @@ -212,7 +223,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters, if (virNWFilterObjWantRemoved(obj)) { virReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } @@ -247,7 +258,7 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, if (obj) { rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) break; } @@ -329,10 +340,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, _("filter with same UUID but different name " "('%s') already exists"), objdef->name); - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); } else { if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -342,7 +353,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, virReportError(VIR_ERR_OPERATION_FAILED, _("filter '%s' already exists with uuid %s"), def->name, uuidstr); - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } } @@ -367,7 +378,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild() < 0) { obj->newDef = NULL; - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return NULL; } @@ -382,10 +393,10 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj) < 0) { - virObjectUnlock(obj); - virObjectUnref(obj); + virNWFilterObjEndAPI(&obj); return NULL; } + virObjectRef(obj); obj->def = def; return obj; @@ -558,8 +569,7 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, continue; obj = virNWFilterObjListLoadConfig(nwfilters, configDir, entry->d_name); - if (obj) - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); } VIR_DIR_CLOSE(dir); diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 9faba264a..0281bc5f5 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); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 67e3ade0f..edda56f80 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1039,6 +1039,7 @@ virNodeDeviceObjListRemove; # conf/virnwfilterobj.h +virNWFilterObjEndAPI; virNWFilterObjGetDef; virNWFilterObjGetNewDef; virNWFilterObjListAssignDef; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index ffd603d70..c9bbae422 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -466,7 +466,7 @@ nwfilterLookupByUUID(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } @@ -496,7 +496,7 @@ nwfilterLookupByName(virConnectPtr conn, nwfilter = virGetNWFilter(conn, def->name, def->uuid); cleanup: - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return nwfilter; } @@ -590,8 +590,7 @@ nwfilterDefineXML(virConnectPtr conn, cleanup: virNWFilterDefFree(def); - if (obj) - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); @@ -629,13 +628,10 @@ nwfilterUndefine(virNWFilterPtr nwfilter) goto cleanup; virNWFilterObjListRemove(driver->nwfilters, obj); - obj = NULL; ret = 0; cleanup: - if (obj) - virObjectUnlock(obj); - + virNWFilterObjEndAPI(&obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(); @@ -667,7 +663,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, ret = virNWFilterDefFormat(def); cleanup: - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 106364c67..48d0e1769 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -316,7 +316,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst) size_t i; for (i = 0; i < inst->nfilters; i++) - virObjectUnlock(inst->filters[i]); + virNWFilterObjEndAPI(&inst->filters[i]); VIR_FREE(inst->filters); inst->nfilters = 0; @@ -426,8 +426,7 @@ virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver, if (ret < 0) virNWFilterInstReset(inst); virNWFilterHashTableFree(tmpvars); - if (obj) - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return ret; } @@ -541,7 +540,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, vars))) { - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); return -1; } @@ -565,7 +564,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterHashTableFree(tmpvars); - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); if (rc < 0) return -1; } @@ -839,7 +838,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, virNWFilterHashTableFree(vars1); err_exit: - virObjectUnlock(obj); + virNWFilterObjEndAPI(&obj); VIR_FREE(str_ipaddr); VIR_FREE(str_macaddr); -- 2.13.6
Based-on-work-of: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 1 - src/conf/virdomainobjlist.c | 3 +- src/conf/virnwfilterobj.c | 409 +++++++++++++++++++++++++++-------------- src/conf/virnwfilterobj.h | 3 - src/libvirt_private.syms | 1 - src/nwfilter/nwfilter_driver.c | 4 +- 6 files changed, 279 insertions(+), 142 deletions(-) diff --git a/cfg.mk b/cfg.mk index 78f805b27..89779fb05 100644 --- a/cfg.mk +++ b/cfg.mk @@ -242,7 +242,6 @@ useless_free_options = \ # y virNWFilterIncludeDefFree # n virNWFilterFreeName (returns int) # y virNWFilterObjFree -# n virNWFilterObjListFree FIXME # y virNWFilterRuleDefFree # n virNWFilterRuleFreeInstanceData (typedef) # y virNWFilterRuleInstFree diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 87a742b1e..4d3cc94b3 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -206,7 +206,8 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, virObjectRWLockRead(doms); obj = virHashLookup(doms->objsName, name); - virObjectRef(obj); + if (obj) + virObjectRef(obj); virObjectRWUnlock(doms); if (obj) { virObjectLock(obj); diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 6a54628b6..bb4d0a036 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -43,11 +43,20 @@ struct _virNWFilterObj { }; static virClassPtr virNWFilterObjClass; +static virClassPtr virNWFilterObjListClass; static void virNWFilterObjDispose(void *obj); +static void virNWFilterObjListDispose(void *obj); struct _virNWFilterObjList { - size_t count; - virNWFilterObjPtr *objs; + virObjectRWLockable parent; + + /* uuid string -> virNWFilterObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; + + /* name -> virNWFilterObj mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; }; static int virNWFilterObjOnceInit(void) @@ -58,6 +67,12 @@ static int virNWFilterObjOnceInit(void) virNWFilterObjDispose))) return -1; + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(), + "virNWFilterObjList", + sizeof(virNWFilterObjList), + virNWFilterObjListDispose))) + return -1; + return 0; } @@ -123,14 +138,14 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj) } -void -virNWFilterObjListFree(virNWFilterObjListPtr nwfilters) +static void +virNWFilterObjListDispose(void *obj) { - size_t i; - for (i = 0; i < nwfilters->count; i++) - virObjectUnref(nwfilters->objs[i]); - VIR_FREE(nwfilters->objs); - VIR_FREE(nwfilters); + + virNWFilterObjListPtr nwfilters = obj; + + virHashFree(nwfilters->objs); + virHashFree(nwfilters->objsName); } @@ -139,8 +154,18 @@ virNWFilterObjListNew(void) { virNWFilterObjListPtr nwfilters; - if (VIR_ALLOC(nwfilters) < 0) + if (virNWFilterObjInitialize() < 0) return NULL; + + if (!(nwfilters = virObjectRWLockableNew(virNWFilterObjListClass))) + return NULL; + + if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData)) || + !(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) { + virObjectUnref(nwfilters); + return NULL; + } + return nwfilters; } @@ -149,20 +174,35 @@ void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj) { - size_t i; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(obj->def->uuid, uuidstr); + virObjectRef(obj); virObjectUnlock(obj); - for (i = 0; i < nwfilters->count; i++) { - virObjectLock(nwfilters->objs[i]); - if (nwfilters->objs[i] == obj) { - virNWFilterObjEndAPI(&nwfilters->objs[i]); + virObjectRWLockWrite(nwfilters); + virObjectLock(obj); + virHashRemoveEntry(nwfilters->objs, uuidstr); + virHashRemoveEntry(nwfilters->objsName, obj->def->name); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(nwfilters); +} + + +static virNWFilterObjPtr +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters, + const unsigned char *uuid) +{ + virNWFilterObjPtr obj = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(uuid, uuidstr); - VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); - break; - } - virObjectUnlock(nwfilters->objs[i]); - } + obj = virHashLookup(nwfilters->objs, uuidstr); + if (obj) + virObjectRef(obj); + return obj; } @@ -170,20 +210,27 @@ virNWFilterObjPtr virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters, const unsigned char *uuid) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectRWLockRead(nwfilters); + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuid); + virObjectRWUnlock(nwfilters); + if (obj) virObjectLock(obj); - def = obj->def; - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN)) - return virObjectRef(obj); - virObjectUnlock(obj); - } + return obj; +} - return NULL; + +static virNWFilterObjPtr +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters, + const char *name) +{ + virNWFilterObjPtr obj; + + obj = virHashLookup(nwfilters->objsName, name); + if (obj) + virObjectRef(obj); + return obj; } @@ -191,20 +238,14 @@ virNWFilterObjPtr virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters, const char *name) { - size_t i; virNWFilterObjPtr obj; - virNWFilterDefPtr def; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; + virObjectRWLockRead(nwfilters); + obj = virNWFilterObjListFindByNameLocked(nwfilters, name); + virObjectRWUnlock(nwfilters); + if (obj) virObjectLock(obj); - def = obj->def; - if (STREQ_NULLABLE(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - - return NULL; + return obj; } @@ -253,9 +294,10 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters, break; } - obj = virNWFilterObjListFindByName(nwfilters, - entry->include->filterref); + obj = virNWFilterObjListFindByNameLocked(nwfilters, + entry->include->filterref); if (obj) { + virObjectLock(obj); rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def, filtername); virNWFilterObjEndAPI(&obj); @@ -325,51 +367,52 @@ virNWFilterDefEqual(const virNWFilterDef *def1, } -virNWFilterObjPtr -virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, - virNWFilterDefPtr def) +static virNWFilterObjPtr +virNWFilterObjListAssignDefLocked(virNWFilterObjListPtr nwfilters, + virNWFilterDefPtr def) { - virNWFilterObjPtr obj; - virNWFilterDefPtr objdef; + virNWFilterObjPtr obj = NULL; + virNWFilterObjPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) { - objdef = obj->def; + virUUIDFormat(def->uuid, uuidstr); - if (STRNEQ(def->name, objdef->name)) { + if ((obj = virNWFilterObjListFindByUUIDLocked(nwfilters, def->uuid))) { + virObjectLock(obj); + + if (STRNEQ(def->name, obj->def->name)) { virReportError(VIR_ERR_OPERATION_FAILED, _("filter with same UUID but different name " "('%s') already exists"), - objdef->name); - virNWFilterObjEndAPI(&obj); - return NULL; + obj->def->name); + goto cleanup; } - virNWFilterObjEndAPI(&obj); } else { - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virObjectLock(obj); - objdef = obj->def; - virUUIDFormat(objdef->uuid, uuidstr); + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, - _("filter '%s' already exists with uuid %s"), + _("filter '%s' already exists with UUID %s"), def->name, uuidstr); - virNWFilterObjEndAPI(&obj); - return NULL; + goto cleanup; } } + virNWFilterObjEndAPI(&obj); + if (virNWFilterObjListDefLoopDetect(nwfilters, def) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("filter would introduce a loop")); - return NULL; + goto cleanup; } - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { + virObjectLock(obj); - objdef = obj->def; - if (virNWFilterDefEqual(def, objdef)) { - virNWFilterDefFree(objdef); + if (virNWFilterDefEqual(def, obj->def)) { + virNWFilterDefFree(obj->def); obj->def = def; return obj; } @@ -382,7 +425,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, return NULL; } - virNWFilterDefFree(objdef); + virNWFilterDefFree(obj->def); obj->def = def; obj->newDef = NULL; return obj; @@ -391,35 +434,114 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, if (!(obj = virNWFilterObjNew())) return NULL; - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs, - nwfilters->count, obj) < 0) { - virNWFilterObjEndAPI(&obj); - return NULL; + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0) + goto cleanup; + + if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) { + virHashRemoveEntry(nwfilters->objs, uuidstr); + goto cleanup; } virObjectRef(obj); + + /* Increase refcounter again. We need two references for the + * hash tables above and one to return to the caller. */ + virObjectRef(obj); obj->def = def; + ret = obj; + obj = NULL; + + cleanup: + virNWFilterObjEndAPI(&obj); + return ret; +} + + +virNWFilterObjPtr +virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, + virNWFilterDefPtr def) +{ + virNWFilterObjPtr obj; + + virObjectRWLockWrite(nwfilters); + obj = virNWFilterObjListAssignDefLocked(nwfilters, def); + virObjectRWUnlock(nwfilters); return obj; } +struct virNWFilterObjListData { + virNWFilterObjListFilter filter; + virConnectPtr conn; + int count; +}; + + +static int +virNWFilterObjListCount(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + struct virNWFilterObjListData *data = opaque; + + virObjectLock(obj); + if (!data->filter || + data->filter(data->conn, obj->def)) + data->count++; + virObjectUnlock(obj); + return 0; +} + + int virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters, virConnectPtr conn, virNWFilterObjListFilter filter) { - size_t i; - int nfilters = 0; - - for (i = 0; i < nwfilters->count; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virObjectLock(obj); - if (!filter || filter(conn, obj->def)) - nfilters++; - virObjectUnlock(obj); + struct virNWFilterObjListData data = { filter, conn, 0 }; + + virObjectRWLockRead(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListCount, &data); + virObjectRWUnlock(nwfilters); + return data.count; +} + + +struct virNWFilterNameData { + virNWFilterObjListFilter filter; + virConnectPtr conn; + int oom; + int numnames; + int maxnames; + char **const names; +}; + + +static int +virNWFilterObjListCopyNames(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNWFilterObjPtr obj = payload; + struct virNWFilterNameData *data = opaque; + + if (data->oom) + return 0; + + virObjectLock(obj); + if (data->filter && + !data->filter(data->conn, obj->def)) + goto cleanup; + if (data->numnames < data->maxnames) { + if (VIR_STRDUP(data->names[data->numnames], obj->def->name) < 0) + data->oom = 1; + else + data->numnames++; } - - return nfilters; + cleanup: + virObjectUnlock(obj); + return 0; } @@ -430,31 +552,64 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, char **const names, int maxnames) { - int nnames = 0; + struct virNWFilterNameData data = {filter, conn, 0, 0, maxnames, names}; size_t i; - virNWFilterDefPtr def; - - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virObjectLock(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virObjectUnlock(obj); - goto failure; - } - nnames++; - } - virObjectUnlock(obj); + + virObjectRWLockRead(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListCopyNames, &data); + virObjectRWUnlock(nwfilters); + if (data.oom) { + for (i = 0; i < data.numnames; i++) + VIR_FREE(data.names[i]); + return -1; + } + + return data.numnames; +} + + +struct virNWFilterListData { + virConnectPtr conn; + virNWFilterPtr *nwfilters; + virNWFilterObjListFilter filter; + int nnwfilters; + bool error; +}; + + +static int +virNWFilterObjListPopulate(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNWFilterListData *data = opaque; + virNWFilterObjPtr obj = payload; + virNWFilterPtr nwfilter = NULL; + + if (data->error) + return 0; + + virObjectLock(obj); + + if (data->filter && + !data->filter(data->conn, obj->def)) + goto cleanup; + + if (!data->nwfilters) { + data->nnwfilters++; + goto cleanup; } - return nnames; + if (!(nwfilter = virGetNWFilter(data->conn, obj->def->name, obj->def->uuid))) { + data->error = true; + goto cleanup; + } - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + data->nwfilters[data->nnwfilters++] = nwfilter; - return -1; + cleanup: + virObjectUnlock(obj); + return 0; } @@ -464,47 +619,33 @@ virNWFilterObjListExport(virConnectPtr conn, virNWFilterPtr **filters, virNWFilterObjListFilter filter) { - virNWFilterPtr *tmp_filters = NULL; - int nfilters = 0; - virNWFilterPtr nwfilter = NULL; - virNWFilterObjPtr obj = NULL; - virNWFilterDefPtr def; - size_t i; int ret = -1; + struct virNWFilterListData data = {.conn = conn, .nwfilters = NULL, + .filter = filter, .nnwfilters = 0, .error = false}; - if (!filters) { - ret = nwfilters->count; + virObjectRWLockRead(nwfilters); + if (filters && VIR_ALLOC_N(data.nwfilters, virHashSize(nwfilters->objs) + 1) < 0) goto cleanup; - } - if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0) + virHashForEach(nwfilters->objs, virNWFilterObjListPopulate, &data); + + if (data.error) goto cleanup; - for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; - virObjectLock(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) { - virObjectUnlock(obj); - goto cleanup; - } - tmp_filters[nfilters++] = nwfilter; - } - virObjectUnlock(obj); + if (data.nnwfilters) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data.nwfilters, data.nnwfilters + 1)); + *filters = data.nwfilters; + data.nwfilters = NULL; } - *filters = tmp_filters; - tmp_filters = NULL; - ret = nfilters; - + ret = data.nnwfilters; cleanup: - if (tmp_filters) { - for (i = 0; i < nfilters; i ++) - virObjectUnref(tmp_filters[i]); - } - VIR_FREE(tmp_filters); + virObjectRWUnlock(nwfilters); + while (data.nwfilters && data.nnwfilters) + virObjectUnref(data.nwfilters[--data.nnwfilters]); + VIR_FREE(data.nwfilters); return ret; } diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 0281bc5f5..caff76e9a 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -56,9 +56,6 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj); virNWFilterObjListPtr virNWFilterObjListNew(void); -void -virNWFilterObjListFree(virNWFilterObjListPtr nwfilters); - void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index edda56f80..fe63defb3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1047,7 +1047,6 @@ virNWFilterObjListExport; virNWFilterObjListFindByName; virNWFilterObjListFindByUUID; virNWFilterObjListFindInstantiateFilter; -virNWFilterObjListFree; virNWFilterObjListGetNames; virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index c9bbae422..093844c9e 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -270,7 +270,7 @@ nwfilterStateInitialize(bool privileged, virNWFilterIPAddrMapShutdown(); err_free_driverstate: - virNWFilterObjListFree(driver->nwfilters); + virObjectUnref(driver->nwfilters); VIR_FREE(driver); return -1; @@ -354,7 +354,7 @@ nwfilterStateCleanup(void) } /* free inactive nwfilters */ - virNWFilterObjListFree(driver->nwfilters); + virObjectUnref(driver->nwfilters); virMutexDestroy(&driver->lock); VIR_FREE(driver); -- 2.13.6
On 02/12/2018 05:52 AM, Michal Privoznik wrote:
@@ -430,31 +552,64 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters, char **const names, int maxnames) { - int nnames = 0; + struct virNWFilterNameData data = {filter, conn, 0, 0, maxnames, names};
Here you don't user .filter = filter while further below you. The rest looks all good to me.
size_t i; - virNWFilterDefPtr def; - - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) { - virNWFilterObjPtr obj = nwfilters->objs[i]; - virObjectLock(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virObjectUnlock(obj); - goto failure; - } - nnames++; - } - virObjectUnlock(obj); + + virObjectRWLockRead(nwfilters); + virHashForEach(nwfilters->objs, virNWFilterObjListCopyNames, &data); + virObjectRWUnlock(nwfilters); + if (data.oom) { + for (i = 0; i < data.numnames; i++) + VIR_FREE(data.names[i]); + return -1; + } + + return data.numnames; +} + + +struct virNWFilterListData { + virConnectPtr conn; + virNWFilterPtr *nwfilters; + virNWFilterObjListFilter filter; + int nnwfilters; + bool error; +}; + + +static int +virNWFilterObjListPopulate(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNWFilterListData *data = opaque; + virNWFilterObjPtr obj = payload; + virNWFilterPtr nwfilter = NULL; + + if (data->error) + return 0; + + virObjectLock(obj); + + if (data->filter && + !data->filter(data->conn, obj->def)) + goto cleanup; + + if (!data->nwfilters) { + data->nnwfilters++; + goto cleanup; }
- return nnames; + if (!(nwfilter = virGetNWFilter(data->conn, obj->def->name, obj->def->uuid))) { + data->error = true; + goto cleanup; + }
- failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + data->nwfilters[data->nnwfilters++] = nwfilter;
- return -1; + cleanup: + virObjectUnlock(obj); + return 0; }
@@ -464,47 +619,33 @@ virNWFilterObjListExport(virConnectPtr conn, virNWFilterPtr **filters, virNWFilterObjListFilter filter) { - virNWFilterPtr *tmp_filters = NULL; - int nfilters = 0; - virNWFilterPtr nwfilter = NULL; - virNWFilterObjPtr obj = NULL; - virNWFilterDefPtr def; - size_t i; int ret = -1; + struct virNWFilterListData data = {.conn = conn, .nwfilters = NULL, + .filter = filter, .nnwfilters = 0, .error = false};
- if (!filters) { - ret = nwfilters->count; + virObjectRWLockRead(nwfilters); + if (filters && VIR_ALLOC_N(data.nwfilters, virHashSize(nwfilters->objs) + 1) < 0) goto cleanup; - }
- if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0) + virHashForEach(nwfilters->objs, virNWFilterObjListPopulate, &data); + + if (data.error) goto cleanup;
- for (i = 0; i < nwfilters->count; i++) { - obj = nwfilters->objs[i]; - virObjectLock(obj); - def = obj->def; - if (!filter || filter(conn, def)) { - if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) { - virObjectUnlock(obj); - goto cleanup; - } - tmp_filters[nfilters++] = nwfilter; - } - virObjectUnlock(obj); + if (data.nnwfilters) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data.nwfilters, data.nnwfilters + 1)); + *filters = data.nwfilters; + data.nwfilters = NULL; }
- *filters = tmp_filters; - tmp_filters = NULL; - ret = nfilters; - + ret = data.nnwfilters; cleanup: - if (tmp_filters) { - for (i = 0; i < nfilters; i ++) - virObjectUnref(tmp_filters[i]); - } - VIR_FREE(tmp_filters); + virObjectRWUnlock(nwfilters); + while (data.nwfilters && data.nnwfilters) + virObjectUnref(data.nwfilters[--data.nnwfilters]);
+ VIR_FREE(data.nwfilters); return ret; }
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 0281bc5f5..caff76e9a 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -56,9 +56,6 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj); virNWFilterObjListPtr virNWFilterObjListNew(void);
-void -virNWFilterObjListFree(virNWFilterObjListPtr nwfilters); - void virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters, virNWFilterObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index edda56f80..fe63defb3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1047,7 +1047,6 @@ virNWFilterObjListExport; virNWFilterObjListFindByName; virNWFilterObjListFindByUUID; virNWFilterObjListFindInstantiateFilter; -virNWFilterObjListFree; virNWFilterObjListGetNames; virNWFilterObjListLoadAllConfigs; virNWFilterObjListNew; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index c9bbae422..093844c9e 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -270,7 +270,7 @@ nwfilterStateInitialize(bool privileged, virNWFilterIPAddrMapShutdown();
err_free_driverstate: - virNWFilterObjListFree(driver->nwfilters); + virObjectUnref(driver->nwfilters); VIR_FREE(driver);
return -1; @@ -354,7 +354,7 @@ nwfilterStateCleanup(void) }
/* free inactive nwfilters */ - virNWFilterObjListFree(driver->nwfilters); + virObjectUnref(driver->nwfilters);
virMutexDestroy(&driver->lock); VIR_FREE(driver);
From: John Ferlan <jferlan@redhat.com> Now that nwfilters object list is self locking, it's no longer necessary to hold the driver level lock for certain API's. Signed-off-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/nwfilter/nwfilter_driver.c | 47 +++++++++++------------------------------- 1 file changed, 12 insertions(+), 35 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 093844c9e..d49c32e4c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -295,6 +295,10 @@ nwfilterStateReload(void) /* shut down all threads -- they will be restarted if necessary */ virNWFilterLearnThreadsTerminate(true); + /* Serialization of virNWFilterObjListLoadAllConfigs is extremely + * important as it relates to virNWFilterObjListFindInstantiateFilter + * processing via virNWFilterTriggerVMFilterRebuild that occurs during + * virNWFilterObjListAssignDef */ nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); @@ -452,11 +456,7 @@ nwfilterLookupByUUID(virConnectPtr conn, virNWFilterDefPtr def; virNWFilterPtr nwfilter = NULL; - nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(uuid); - nwfilterDriverUnlock(); - - if (!obj) + if (!(obj = nwfilterObjFromNWFilter(uuid))) return NULL; def = virNWFilterObjGetDef(obj); @@ -479,11 +479,7 @@ nwfilterLookupByName(virConnectPtr conn, virNWFilterDefPtr def; virNWFilterPtr nwfilter = NULL; - nwfilterDriverLock(); - obj = virNWFilterObjListFindByName(driver->nwfilters, name); - nwfilterDriverUnlock(); - - if (!obj) { + if (!(obj = virNWFilterObjListFindByName(driver->nwfilters, name))) { virReportError(VIR_ERR_NO_NWFILTER, _("no nwfilter with matching name '%s'"), name); return NULL; @@ -517,17 +513,12 @@ nwfilterConnectListNWFilters(virConnectPtr conn, char **const names, int maxnames) { - int nnames; - if (virConnectListNWFiltersEnsureACL(conn) < 0) return -1; - nwfilterDriverLock(); - nnames = virNWFilterObjListGetNames(driver->nwfilters, conn, - virConnectListNWFiltersCheckACL, - names, maxnames); - nwfilterDriverUnlock(); - return nnames; + return virNWFilterObjListGetNames(driver->nwfilters, conn, + virConnectListNWFiltersCheckACL, + names, maxnames); } @@ -536,19 +527,13 @@ nwfilterConnectListAllNWFilters(virConnectPtr conn, virNWFilterPtr **nwfilters, unsigned int flags) { - int ret; - virCheckFlags(0, -1); if (virConnectListAllNWFiltersEnsureACL(conn) < 0) return -1; - nwfilterDriverLock(); - ret = virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, - virConnectListAllNWFiltersCheckACL); - nwfilterDriverUnlock(); - - return ret; + return virNWFilterObjListExport(conn, driver->nwfilters, nwfilters, + virConnectListAllNWFiltersCheckACL); } static virNWFilterPtr @@ -566,7 +551,6 @@ nwfilterDefineXML(virConnectPtr conn, return NULL; } - nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); @@ -594,7 +578,6 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); - nwfilterDriverUnlock(); return nwfilter; } @@ -606,7 +589,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterDefPtr def; int ret = -1; - nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); virNWFilterCallbackDriversLock(); @@ -634,7 +616,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterObjEndAPI(&obj); virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); - nwfilterDriverUnlock(); return ret; } @@ -649,11 +630,7 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, virCheckFlags(0, NULL); - nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(nwfilter->uuid); - nwfilterDriverUnlock(); - - if (!obj) + if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) return NULL; def = virNWFilterObjGetDef(obj); -- 2.13.6
participants (5)
-
John Ferlan -
Michal Privoznik -
Pavel Hrdina -
Peter Krempa -
Stefan Berger