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 <pmores@xxxxxxxxxx> 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
