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 ...

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

/* Feature flags for the architecture info */ static const struct qemu_feature_flags const arch_info_i686_flags [] = { @@ -1769,6 +1783,8 @@ qemuEmulatorCachedInfoGet(enum qemuCapsP bool alreadyCached; int i;

+ virMutexLock(&qemuEmulatorCacheMutex); + if (stat(binary, &st) != 0) { char ebuf[1024]; VIR_INFO("Failed to stat emulator %s : %s", Index: libvirt-0.9.10/src/qemu/qemu_driver.c =================================================================== --- libvirt-0.9.10.orig/src/qemu/qemu_driver.c +++ libvirt-0.9.10/src/qemu/qemu_driver.c @@ -585,6 +585,9 @@ qemudStartup(int privileged) { if (qemuSecurityInit(qemu_driver) < 0) goto error;

+ if (qemuCapsCacheInit() < 0) + goto error; + if ((qemu_driver->caps = qemuCreateCapabilities(NULL, qemu_driver)) == NULL) goto error; Index: libvirt-0.9.10/src/qemu/qemu_capabilities.h =================================================================== --- libvirt-0.9.10.orig/src/qemu/qemu_capabilities.h +++ libvirt-0.9.10/src/qemu/qemu_capabilities.h @@ -139,6 +139,8 @@ void qemuCapsClear(virBitmapPtr caps, bool qemuCapsGet(virBitmapPtr caps, enum qemuCapsFlags flag);

+int qemuCapsCacheInit(void); + virCapsPtr qemuCapsInit(virCapsPtr old_caps);

int qemuCapsProbeMachineTypes(const char *binary,

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, 2012-03-12 at 10:16 -0400, Lee Schermerhorn wrote:

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?

Update: 'make check' tests [qemuxml2argv] succeed with static initializer removed. Previous failures were apparently either an artifact of the 0.8.8-1ubuntu6.x code base or a defect elsewhere in my patches at the time. I was still debugging then, trying to get tests to pass. Question whether the mutex is needed still stands, but since the driver lock is RW, it probably is. cache mutex could probably be made RW as well, and only taken for W when filling/refreshing the cache. I'm running more tests and will update the series when/if I hear that it's worth doing so. Regards, Lee

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