[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH RFC 8/8] add qemu cache mutex



On Mon, 2012-03-12 at 14:00 +0100, Michal Privoznik wrote:
> On 11.03.2012 19:56, Lee Schermerhorn wrote:
> > Add a mutex for access to the qemu emulator cache.  Not clear that
> > this is actually needed -- driver should be locked across calls [?].
> > The patch can be dropped if not needed.
> > ---
> >  src/qemu/qemu_capabilities.c |   18 +++++++++++++++++-
> >  src/qemu/qemu_capabilities.h |    2 ++
> >  src/qemu/qemu_driver.c       |    3 +++
> >  3 files changed, 22 insertions(+), 1 deletion(-)
> > 
> > Index: libvirt-0.9.10/src/qemu/qemu_capabilities.c
> > ===================================================================
> > --- libvirt-0.9.10.orig/src/qemu/qemu_capabilities.c
> > +++ libvirt-0.9.10/src/qemu/qemu_capabilities.c
> > @@ -27,6 +27,7 @@
> >  #include "memory.h"
> >  #include "logging.h"
> >  #include "virterror_internal.h"
> > +#include "threads.h"
> >  #include "util.h"
> >  #include "virfile.h"
> >  #include "nodeinfo.h"
> > @@ -180,6 +181,11 @@ enum qemuCapsProbes {
> >      QEMU_PROBE_SIZE
> >  };
> >  
> > +/*
> > + * Use static initializer for tests
> > + */
> > +static virMutex qemuEmulatorCacheMutex = { PTHREAD_MUTEX_INITIALIZER };
> 
> This is not allowed in our code as we build with win32 threads which
> initialize mutexes completely different. Why do you want to initialize
> it here anyway ...


Thanks.  I didn't know that.

As the comment says, I added it for the internal tests.  It appeared to
me that they don't go through the driver init code where I inserted the
call to to the init function below.  The tests were hanging at the first
attempt to acquire the mutex.  It could have been a defect in my patches
at that time [that may still be there?].  When I inserted the static
initializer, the tests passed.  That was back on the 0.8.8 ubuntu natty
code base.  I'll pull it out and see if they pass w/o it, now that the
tests all seem to pass otherwise.  They certainly pass w/o this patch
applied, but they're all single threaded, right?

Bigger question is:  is the mutex actually needed at all?  I.e., can I
assume that the driver is always locked -- in practice, not necessarily
for the tests -- when the probes are called?

Lee

> > +
> >  typedef struct _qemuEmulatorCache qemuEmulatorCache;
> >  typedef qemuEmulatorCache* qemuEmulatorCachePtr;
> >  struct _qemuEmulatorCache {
> > @@ -206,9 +212,17 @@ qemuEmulatorCachedInfoGet(enum qemuCapsP
> >                            const char *binary,
> >                            const char *arch);
> >  
> > +int
> > +qemuCapsCacheInit(void)
> > +{
> > +    return virMutexInit(&qemuEmulatorCacheMutex);
> > +}
> > +
> 
> if you have created this function?
> >  static void
> >  qemuEmulatorCachedInfoRelease(qemuEmulatorCachePtr emulator ATTRIBUTE_UNUSED)
> > -{ }
> > +{
> > +    virMutexUnlock(&qemuEmulatorCacheMutex);
> > +}
> >  



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]