Re: [PATCH 02/43] bhyve: convert virMutex to GMutex
On Wed, Apr 15, 2020 at 01:59:46PM +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 15, 2020 at 02:15:35PM +0200, Pavel Mores wrote:
> > On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> > > On Tue, Apr 14, 2020 at 6:06 PM Pavel Mores wrote:
> > > >
> > > > By the way, the approach taken here with bhyveDriver{Lock,Unlock}()
> > > > might make
> > > > sense with the whole series - implement e.g. virMutexInit() in terms of
> > > > g_mutex_init() in the first phase and only then replace the actual
> > > > virMutexInit() calls if considered beneficial...
> > >
> > > So you mean one patch doing 's/virMutex/GMutex' and then inside
> > > virMutex*() we call the g_mutex_*() equivalent? And maybe make
> > > virMutex*() `inline`?
> >
> > Yes - I mean, I'm not familiar enough with this to be sure off-hand that
> > just
> > doing a literal find & replace would work with no undesired side-effects,
> > but
> > conceptually yes, that's the idea.
> >
> > That's just a thought though - taking that approach would have broken the
> > refactor into two more manageable & testable chunks but seeing as you've
> > done
> > the hard work already, there's no need to rework the series just because of
> > me.
> > :-)
>
> Replacing the virMutex calls with GMutex APis in all callers is the
> desirable approach. The goal of using GLib APIs is to remove any
> libvirt specfic APIs which duplicate GLib. Thus re-writing virMutex
> APIs impls to call GMutex is just delaying the desired end state where
> virMutex ceases to exist.
Sure, I agree with the goal, no question about that. I was just thinking about
getting there in two steps (with no delay between them implied) which might
have been at least easier to review. Also bisecting would be easier that way
if we later suspect that there might have been an unexpected change of
behaviour after all, possibly related to the migration to glib sync primitives.
Anyway, the work has been pretty much done already so the point seems moot.
Thanks,
pvl
Re: [PATCH 02/43] bhyve: convert virMutex to GMutex
On Wed, Apr 15, 2020 at 02:15:35PM +0200, Pavel Mores wrote:
> On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> > On Tue, Apr 14, 2020 at 6:06 PM Pavel Mores wrote:
> > >
> > > By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might
> > > make
> > > sense with the whole series - implement e.g. virMutexInit() in terms of
> > > g_mutex_init() in the first phase and only then replace the actual
> > > virMutexInit() calls if considered beneficial...
> >
> > So you mean one patch doing 's/virMutex/GMutex' and then inside
> > virMutex*() we call the g_mutex_*() equivalent? And maybe make
> > virMutex*() `inline`?
>
> Yes - I mean, I'm not familiar enough with this to be sure off-hand that just
> doing a literal find & replace would work with no undesired side-effects, but
> conceptually yes, that's the idea.
>
> That's just a thought though - taking that approach would have broken the
> refactor into two more manageable & testable chunks but seeing as you've done
> the hard work already, there's no need to rework the series just because of
> me.
> :-)
Replacing the virMutex calls with GMutex APis in all callers is the
desirable approach. The goal of using GLib APIs is to remove any
libvirt specfic APIs which duplicate GLib. Thus re-writing virMutex
APIs impls to call GMutex is just delaying the desired end state where
virMutex ceases to exist.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 02/43] bhyve: convert virMutex to GMutex
On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> On Tue, Apr 14, 2020 at 6:06 PM Pavel Mores wrote:
> >
> > By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might
> > make
> > sense with the whole series - implement e.g. virMutexInit() in terms of
> > g_mutex_init() in the first phase and only then replace the actual
> > virMutexInit() calls if considered beneficial...
>
> So you mean one patch doing 's/virMutex/GMutex' and then inside
> virMutex*() we call the g_mutex_*() equivalent? And maybe make
> virMutex*() `inline`?
Yes - I mean, I'm not familiar enough with this to be sure off-hand that just
doing a literal find & replace would work with no undesired side-effects, but
conceptually yes, that's the idea.
That's just a thought though - taking that approach would have broken the
refactor into two more manageable & testable chunks but seeing as you've done
the hard work already, there's no need to rework the series just because of me.
:-)
pvl
Re: [PATCH 02/43] bhyve: convert virMutex to GMutex
On Tue, Apr 14, 2020 at 6:06 PM Pavel Mores wrote:
>
> By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might make
> sense with the whole series - implement e.g. virMutexInit() in terms of
> g_mutex_init() in the first phase and only then replace the actual
> virMutexInit() calls if considered beneficial...
So you mean one patch doing 's/virMutex/GMutex' and then inside
virMutex*() we call the g_mutex_*() equivalent? And maybe make
virMutex*() `inline`?
Att.
--
Rafael Fonseca
Re: [PATCH 02/43] bhyve: convert virMutex to GMutex
On Fri, Apr 10, 2020 at 03:54:29PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca
> ---
> src/bhyve/bhyve_driver.c | 11 ---
> src/bhyve/bhyve_utils.h | 2 +-
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index b6204c7fb9..6e9a79cb52 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -73,13 +73,13 @@ bhyveConnPtr bhyve_driver = NULL;
> void
> bhyveDriverLock(bhyveConnPtr driver)
> {
> -virMutexLock(&driver->lock);
> +g_mutex_lock(&driver->lock);
> }
>
> void
> bhyveDriverUnlock(bhyveConnPtr driver)
> {
> -virMutexUnlock(&driver->lock);
> +g_mutex_unlock(&driver->lock);
> }
>
> static int
> @@ -1199,7 +1199,7 @@ bhyveStateCleanup(void)
> if (bhyve_driver->lockFD != -1)
> virPidFileRelease(BHYVE_STATE_DIR, "driver", bhyve_driver->lockFD);
>
> -virMutexDestroy(&bhyve_driver->lock);
> +g_mutex_clear(&bhyve_driver->lock);
> VIR_FREE(bhyve_driver);
>
> return 0;
> @@ -1228,10 +1228,7 @@ bhyveStateInitialize(bool privileged,
> return VIR_DRV_STATE_INIT_ERROR;
>
> bhyve_driver->lockFD = -1;
> -if (virMutexInit(&bhyve_driver->lock) < 0) {
> -VIR_FREE(bhyve_driver);
> -return VIR_DRV_STATE_INIT_ERROR;
> -}
> +g_mutex_init(&bhyve_driver->lock);
>
> if (!(bhyve_driver->closeCallbacks = virCloseCallbacksNew()))
> goto cleanup;
> diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
> index f3e80b6121..a92ecb48c4 100644
> --- a/src/bhyve/bhyve_utils.h
> +++ b/src/bhyve/bhyve_utils.h
> @@ -44,7 +44,7 @@ struct _virBhyveDriverConfig {
> };
>
> struct _bhyveConn {
> -virMutex lock;
> +GMutex lock;
>
> virBhyveDriverConfigPtr config;
>
> --
> 2.25.2
>
>
By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might make
sense with the whole series - implement e.g. virMutexInit() in terms of
g_mutex_init() in the first phase and only then replace the actual
virMutexInit() calls if considered beneficial...
Reviewed-by: Pavel Mores
[PATCH 02/43] bhyve: convert virMutex to GMutex
Signed-off-by: Rafael Fonseca
---
src/bhyve/bhyve_driver.c | 11 ---
src/bhyve/bhyve_utils.h | 2 +-
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index b6204c7fb9..6e9a79cb52 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -73,13 +73,13 @@ bhyveConnPtr bhyve_driver = NULL;
void
bhyveDriverLock(bhyveConnPtr driver)
{
-virMutexLock(&driver->lock);
+g_mutex_lock(&driver->lock);
}
void
bhyveDriverUnlock(bhyveConnPtr driver)
{
-virMutexUnlock(&driver->lock);
+g_mutex_unlock(&driver->lock);
}
static int
@@ -1199,7 +1199,7 @@ bhyveStateCleanup(void)
if (bhyve_driver->lockFD != -1)
virPidFileRelease(BHYVE_STATE_DIR, "driver", bhyve_driver->lockFD);
-virMutexDestroy(&bhyve_driver->lock);
+g_mutex_clear(&bhyve_driver->lock);
VIR_FREE(bhyve_driver);
return 0;
@@ -1228,10 +1228,7 @@ bhyveStateInitialize(bool privileged,
return VIR_DRV_STATE_INIT_ERROR;
bhyve_driver->lockFD = -1;
-if (virMutexInit(&bhyve_driver->lock) < 0) {
-VIR_FREE(bhyve_driver);
-return VIR_DRV_STATE_INIT_ERROR;
-}
+g_mutex_init(&bhyve_driver->lock);
if (!(bhyve_driver->closeCallbacks = virCloseCallbacksNew()))
goto cleanup;
diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
index f3e80b6121..a92ecb48c4 100644
--- a/src/bhyve/bhyve_utils.h
+++ b/src/bhyve/bhyve_utils.h
@@ -44,7 +44,7 @@ struct _virBhyveDriverConfig {
};
struct _bhyveConn {
-virMutex lock;
+GMutex lock;
virBhyveDriverConfigPtr config;
--
2.25.2
