Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
---
 src/conf/nwfilter_ipaddrmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c
index 672a58be3a..3c1927f9ff 100644
--- a/src/conf/nwfilter_ipaddrmap.c
+++ b/src/conf/nwfilter_ipaddrmap.c
@@ -32,7 +32,7 @@
 
 #define VIR_FROM_THIS VIR_FROM_NWFILTER
 
-static virMutex ipAddressMapLock = VIR_MUTEX_INITIALIZER;
+G_LOCK_DEFINE_STATIC(ipAddressMapLock);
 static virHashTablePtr ipAddressMap;
 
 
@@ -55,7 +55,7 @@ virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr)
 
     addrCopy = g_strdup(addr);
 
-    virMutexLock(&ipAddressMapLock);
+    G_LOCK(ipAddressMapLock);
 
     val = virHashLookup(ipAddressMap, ifname);
     if (!val) {
@@ -76,7 +76,7 @@ virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr)
     ret = 0;
 
  cleanup:
-    virMutexUnlock(&ipAddressMapLock);
+    G_UNLOCK(ipAddressMapLock);
     VIR_FREE(addrCopy);
 
     return ret;
@@ -102,7 +102,7 @@ virNWFilterIPAddrMapDelIPAddr(const char *ifname, const char *ipaddr)
     int ret = -1;
     virNWFilterVarValuePtr val = NULL;
 
-    virMutexLock(&ipAddressMapLock);
+    G_LOCK(ipAddressMapLock);
 
     if (ipaddr != NULL) {
         val = virHashLookup(ipAddressMap, ifname);
@@ -121,7 +121,7 @@ virNWFilterIPAddrMapDelIPAddr(const char *ifname, const char *ipaddr)
         ret = 0;
     }
 
-    virMutexUnlock(&ipAddressMapLock);
+    G_UNLOCK(ipAddressMapLock);
 
     return ret;
 }
@@ -137,11 +137,11 @@ virNWFilterIPAddrMapGetIPAddr(const char *ifname)
 {
     virNWFilterVarValuePtr res;
 
-    virMutexLock(&ipAddressMapLock);
+    G_LOCK(ipAddressMapLock);
 
     res = virHashLookup(ipAddressMap, ifname);
 
-    virMutexUnlock(&ipAddressMapLock);
+    G_UNLOCK(ipAddressMapLock);
 
     return res;
 }
-- 
2.25.2

On 4/10/20 9:54 AM, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
> ---
>   src/conf/nwfilter_ipaddrmap.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c
> index 672a58be3a..3c1927f9ff 100644
> --- a/src/conf/nwfilter_ipaddrmap.c
> +++ b/src/conf/nwfilter_ipaddrmap.c
> @@ -32,7 +32,7 @@
>   
>   #define VIR_FROM_THIS VIR_FROM_NWFILTER
>   
> -static virMutex ipAddressMapLock = VIR_MUTEX_INITIALIZER;
> +G_LOCK_DEFINE_STATIC(ipAddressMapLock);

The documentation for the G_LOCK() macros says that the name provided in 
the args will be mangled, so you can just use the same name as the 
object you're going to lock, e.g.:


G_LOCK_DEFINE_STATIC(ipAddressMap);


instead of "ipAddressMapLock". The upside is that's less typing, and if 
you do a keyword search you'll find all the places where ipAddressMap is 
locked/unlocked along with its uses. The downside would be that you 
couldn't as easily do a keyword search for just the lock manipulation 
(you'd need to use a regex to search for "G_.*ipAddressMap" or something 
like that); I still kind of like the idea of using the exact same name, 
just because it encourages consistency.


Beyond that, why not just use the G_*() macros for *all* locks in 
libvirt instead of only using them in cases of static locks? It seems 
strange to use the convenience macros in some cases and the raw 
functions in others. (I'm looking at this with 0 experience using the 
Glib locks, so maybe there's something basic I'm just not aware of - 
maybe something necessary is missing from the G_LOCK() macros?).

On 4/10/20 1:01 PM, Laine Stump wrote:
> On 4/10/20 9:54 AM, Rafael Fonseca wrote:
>> Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
>> ---
>>   src/conf/nwfilter_ipaddrmap.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/conf/nwfilter_ipaddrmap.c 
>> b/src/conf/nwfilter_ipaddrmap.c
>> index 672a58be3a..3c1927f9ff 100644
>> --- a/src/conf/nwfilter_ipaddrmap.c
>> +++ b/src/conf/nwfilter_ipaddrmap.c
>> @@ -32,7 +32,7 @@
>>     #define VIR_FROM_THIS VIR_FROM_NWFILTER
>>   -static virMutex ipAddressMapLock = VIR_MUTEX_INITIALIZER;
>> +G_LOCK_DEFINE_STATIC(ipAddressMapLock);
>
> The documentation for the G_LOCK() macros says that the name provided 
> in the args will be mangled, so you can just use the same name as the 
> object you're going to lock, e.g.:
>
>
> G_LOCK_DEFINE_STATIC(ipAddressMap);
>
>
> instead of "ipAddressMapLock". The upside is that's less typing, and 
> if you do a keyword search you'll find all the places where 
> ipAddressMap is locked/unlocked along with its uses. The downside 
> would be that you couldn't as easily do a keyword search for just the 
> lock manipulation (you'd need to use a regex to search for 
> "G_.*ipAddressMap" or something like that); I still kind of like the 
> idea of using the exact same name, just because it encourages 
> consistency.
>
>
> Beyond that, why not just use the G_*() macros for *all* locks in 
> libvirt instead of only using them in cases of static locks? It seems 
> strange to use the convenience macros in some cases and the raw 
> functions in others. (I'm looking at this with 0 experience using the 
> Glib locks, so maybe there's something basic I'm just not aware of - 
> maybe something necessary is missing from the G_LOCK() macros?).


Okay, I already see that the G_LOCK macros don't cover everything - no 
recursive mutexes and no RW mutexes for example. Too bad - it would be 
good to be consistent.

On Fri, Apr 10, 2020 at 7:06 PM Laine Stump <laine@laine.org> wrote:
> > Beyond that, why not just use the G_*() macros for *all* locks in
> > libvirt instead of only using them in cases of static locks? It seems
> > strange to use the convenience macros in some cases and the raw
> > functions in others. (I'm looking at this with 0 experience using the
> > Glib locks, so maybe there's something basic I'm just not aware of -
> > maybe something necessary is missing from the G_LOCK() macros?).
>
>
> Okay, I already see that the G_LOCK macros don't cover everything - no
> recursive mutexes and no RW mutexes for example. Too bad - it would be
> good to be consistent.

Yes, that's one issue. Another is: how do you use those macros with
locks inside structs? You can't do `G_LOCK(obj->parent.lock)` because
it'll result in `g_mutex_lock(&g__obj->parent.lock_lock)` which is
wrong. You'd have to use the raw function
`g_mutex_lock(obj->parent.LOCK_NAME(lock))` anyway, which imho, is
even worse than `g_mutex_lock(&obj->parent.lock)`. The same issue
happens when using mutexes with conditions: `g_cond_wait(cond,
obj->parent.LOCK_NAME(lock))
` instead of just `g_cond_wait(cond, obj->parent.lock)`. So they work
better for statically-defined locks

I don't mind doing whichever you guys prefer, just let me know.


Att.
-- 
Rafael Fonseca

On Sat, Apr 11, 2020 at 08:15:15PM +0200, Rafael Fonseca wrote:
> On Fri, Apr 10, 2020 at 7:06 PM Laine Stump <laine@laine.org> wrote:
> > > Beyond that, why not just use the G_*() macros for *all* locks in
> > > libvirt instead of only using them in cases of static locks? It seems
> > > strange to use the convenience macros in some cases and the raw
> > > functions in others. (I'm looking at this with 0 experience using the
> > > Glib locks, so maybe there's something basic I'm just not aware of -
> > > maybe something necessary is missing from the G_LOCK() macros?).
> >
> >
> > Okay, I already see that the G_LOCK macros don't cover everything - no
> > recursive mutexes and no RW mutexes for example. Too bad - it would be
> > good to be consistent.
> 
> Yes, that's one issue. Another is: how do you use those macros with
> locks inside structs? You can't do `G_LOCK(obj->parent.lock)` because
> it'll result in `g_mutex_lock(&g__obj->parent.lock_lock)` which is
> wrong. You'd have to use the raw function
> `g_mutex_lock(obj->parent.LOCK_NAME(lock))` anyway, which imho, is
> even worse than `g_mutex_lock(&obj->parent.lock)`. The same issue
> happens when using mutexes with conditions: `g_cond_wait(cond,
> obj->parent.LOCK_NAME(lock))
> ` instead of just `g_cond_wait(cond, obj->parent.lock)`. So they work
> better for statically-defined locks
> 
> I don't mind doing whichever you guys prefer, just let me know.

Looking at the source code, the name mangling is pretty much all that the
G_LOCK_DEFINE* macros do.  So - besides some logging - the only advantage to
using them is that you don't have to mangle the lock names manually and can use
names of existing variables as the macros' arguments.

Considering the above, I'd say either use the macros and don't mangle the lock
names in their arguments manually, or don't use the macros.  If consistent
style is a priority I'd lean towards raw functions - unlike the macros, they
can be used everywhere, and having to mangle the lock names by hand doesn't
seem a huge burden to me.  We do loose the logging that the macros do but in my
experience, mutex logging often doesn't turn out as useful in practice as it
might first appear...

	pvl

On 4/15/20 8:43 AM, Pavel Mores wrote:
> On Sat, Apr 11, 2020 at 08:15:15PM +0200, Rafael Fonseca wrote:
>> On Fri, Apr 10, 2020 at 7:06 PM Laine Stump <laine@laine.org> wrote:
>>>> Beyond that, why not just use the G_*() macros for *all* locks in
>>>> libvirt instead of only using them in cases of static locks? It seems
>>>> strange to use the convenience macros in some cases and the raw
>>>> functions in others. (I'm looking at this with 0 experience using the
>>>> Glib locks, so maybe there's something basic I'm just not aware of -
>>>> maybe something necessary is missing from the G_LOCK() macros?).
>>>
>>> Okay, I already see that the G_LOCK macros don't cover everything - no
>>> recursive mutexes and no RW mutexes for example. Too bad - it would be
>>> good to be consistent.
>> Yes, that's one issue. Another is: how do you use those macros with
>> locks inside structs? You can't do `G_LOCK(obj->parent.lock)` because
>> it'll result in `g_mutex_lock(&g__obj->parent.lock_lock)` which is
>> wrong. You'd have to use the raw function
>> `g_mutex_lock(obj->parent.LOCK_NAME(lock))` anyway, which imho, is
>> even worse than `g_mutex_lock(&obj->parent.lock)`. The same issue
>> happens when using mutexes with conditions: `g_cond_wait(cond,
>> obj->parent.LOCK_NAME(lock))
>> ` instead of just `g_cond_wait(cond, obj->parent.lock)`. So they work
>> better for statically-defined locks
>>
>> I don't mind doing whichever you guys prefer, just let me know.
> Looking at the source code, the name mangling is pretty much all that the
> G_LOCK_DEFINE* macros do.  So - besides some logging - the only advantage to
> using them is that you don't have to mangle the lock names manually and can use
> names of existing variables as the macros' arguments.
>
> Considering the above, I'd say either use the macros and don't mangle the lock
> names in their arguments manually, or don't use the macros.  If consistent
> style is a priority I'd lean towards raw functions


I agree with this - consistency is king :-)


>   - unlike the macros, they
> can be used everywhere, and having to mangle the lock names by hand doesn't
> seem a huge burden to me.  We do loose the logging that the macros do but in my
> experience, mutex logging often doesn't turn out as useful in practice as it
> might first appear...
>
> 	pvl
>

On Fri, Apr 10, 2020 at 7:01 PM Laine Stump <laine@laine.org> wrote:
>
> On 4/10/20 9:54 AM, Rafael Fonseca wrote:
> > Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
> > ---
> >   src/conf/nwfilter_ipaddrmap.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c
> > index 672a58be3a..3c1927f9ff 100644
> > --- a/src/conf/nwfilter_ipaddrmap.c
> > +++ b/src/conf/nwfilter_ipaddrmap.c
> > @@ -32,7 +32,7 @@
> >
> >   #define VIR_FROM_THIS VIR_FROM_NWFILTER
> >
> > -static virMutex ipAddressMapLock = VIR_MUTEX_INITIALIZER;
> > +G_LOCK_DEFINE_STATIC(ipAddressMapLock);
>
> The documentation for the G_LOCK() macros says that the name provided in
> the args will be mangled, so you can just use the same name as the
> object you're going to lock, e.g.:
>
>
> G_LOCK_DEFINE_STATIC(ipAddressMap);
>
>
> instead of "ipAddressMapLock". The upside is that's less typing, and if
> you do a keyword search you'll find all the places where ipAddressMap is
> locked/unlocked along with its uses. The downside would be that you
> couldn't as easily do a keyword search for just the lock manipulation
> (you'd need to use a regex to search for "G_.*ipAddressMap" or something
> like that); I still kind of like the idea of using the exact same name,
> just because it encourages consistency.

Ok I can do that.


Att.
-- 
Rafael Fonseca