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);
> +}
>