12:52 p.m.
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
12:52 p.m.
New subject: [libvirt] [PATCH 1/6] virObject: use virReportSystemError if applicable
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(a)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
12:52 p.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
2:10 p.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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.
2:16 p.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn(a)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
10:34 a.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:52 a.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)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
11:08 a.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
>>>> Signed-off-by: Michal Privoznik <mprivozn(a)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.
11:47 a.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
>>>>> Signed-off-by: Michal Privoznik <mprivozn(a)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
3:43 p.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
>>>>>> Signed-off-by: Michal Privoznik <mprivozn(a)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
2:16 p.m.
New subject: [libvirt] [PATCH 2/6] virObject: Introduce virObjectRecursiveLockableNew
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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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);
12:52 p.m.
New subject: [libvirt] [PATCH 3/6] virNWFilterObj: Turn into virObjectLockable
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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
12:52 p.m.
New subject: [libvirt] [PATCH 4/6] conf: Introduce and use virNWFilterObjEndAPI
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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
12:52 p.m.
New subject: [libvirt] [PATCH 5/6] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable
Based-on-work-of: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
11:57 p.m.
New subject: [libvirt] [PATCH 5/6] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable
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);
12:52 p.m.
New subject: [libvirt] [PATCH 6/6] nwfilter: Remove need for nwfilterDriverLock in some API's
From: John Ferlan <jferlan(a)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(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
2540
days inactive
2544
days old
15 comments
5 participants
participants (5)
-
John Ferlan -
Michal Privoznik -
Pavel Hrdina -
Peter Krempa -
Stefan Berger