There are a number of pthreads impls available on Win32 these days, in particular the mingw64 project has a good impl. Delete the native windows thread implementation and rely on using pthreads everywhere. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- configure.ac | 15 +- src/Makefile.am | 4 - src/util/virthread.c | 291 ++++++++++++++++++++++++++++++- src/util/virthread.h | 41 ++++- src/util/virthreadpthread.c | 309 --------------------------------- src/util/virthreadpthread.h | 53 ------ src/util/virthreadwin32.c | 415 -------------------------------------------- src/util/virthreadwin32.h | 57 ------ 8 files changed, 329 insertions(+), 856 deletions(-) delete mode 100644 src/util/virthreadpthread.c delete mode 100644 src/util/virthreadpthread.h delete mode 100644 src/util/virthreadwin32.c delete mode 100644 src/util/virthreadwin32.h diff --git a/configure.ac b/configure.ac index 3a70375..168eb27 100644 --- a/configure.ac +++ b/configure.ac @@ -270,12 +270,21 @@ AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \ posix_memalign prlimit regexec sched_getaffinity setgroups setns \ setrlimit symlink sysctlbyname]) -dnl Availability of pthread functions (if missing, win32 threading is -dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. -dnl LIB_PTHREAD and LIBMULTITHREAD were set during gl_INIT by gnulib. +dnl Availability of pthread functions. Because of $LIB_PTHREAD, we +dnl cannot use AC_CHECK_FUNCS_ONCE. LIB_PTHREAD and LIBMULTITHREAD +dnl were set during gl_INIT by gnulib. old_LIBS=$LIBS LIBS="$LIBS $LIB_PTHREAD $LIBMULTITHREAD" + +pthread_found=yes AC_CHECK_FUNCS([pthread_mutexattr_init]) +AC_CHECK_HEADER([pthread.h],,[pthread_found=no]) + +if test "$ac_cv_func_pthread_mutexattr_init:$pthread_found" != "yes:yes" +then + AC_MSG_ERROR([A pthreads impl is required for building libvirt]) +fi + dnl At least mingw64-winpthreads #defines pthread_sigmask to 0, dnl which in turn causes compilation to complain about unused variables. dnl Expose this broken implementation, so we can work around it. diff --git a/src/Makefile.am b/src/Makefile.am index 997899e..d541883 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -141,8 +141,6 @@ UTIL_SOURCES = \ util/virsysinfo.c util/virsysinfo.h \ util/virsystemd.c util/virsystemd.h \ util/virthread.c util/virthread.h \ - util/virthreadpthread.h \ - util/virthreadwin32.h \ util/virthreadpool.c util/virthreadpool.h \ util/virtime.h util/virtime.c \ util/virtpm.h util/virtpm.c \ @@ -165,8 +163,6 @@ util/virkeymaps.h: $(srcdir)/util/keymaps.csv \ $(AM_V_GEN)$(PYTHON) $(srcdir)/util/virkeycode-mapgen.py \ <$(srcdir)/util/keymaps.csv >$(srcdir)/util/virkeymaps.h -EXTRA_DIST += util/virthreadpthread.c util/virthreadwin32.c - # Internal generic driver infrastructure NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c DATATYPES_SOURCES = datatypes.h datatypes.c diff --git a/src/util/virthread.c b/src/util/virthread.c index dd1768e..b60fb4a 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -23,12 +23,289 @@ #include "virthread.h" -/* On mingw, we prefer native threading over the sometimes-broken - * pthreads-win32 library wrapper. */ -#ifdef WIN32 -# include "virthreadwin32.c" -#elif defined HAVE_PTHREAD_MUTEXATTR_INIT -# include "virthreadpthread.c" +#include <unistd.h> +#include <inttypes.h> +#if HAVE_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif + +#include "viralloc.h" + + +/* Nothing special required for pthreads */ +int virThreadInitialize(void) +{ + return 0; +} + +void virThreadOnExit(void) +{ +} + +int virOnce(virOnceControlPtr once, virOnceFunc init) +{ + return pthread_once(&once->once, init); +} + + +int virMutexInit(virMutexPtr m) +{ + int ret; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); + ret = pthread_mutex_init(&m->lock, &attr); + pthread_mutexattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virMutexInitRecursive(virMutexPtr m) +{ + int ret; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + ret = pthread_mutex_init(&m->lock, &attr); + pthread_mutexattr_destroy(&attr); + if (ret != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virMutexDestroy(virMutexPtr m) +{ + pthread_mutex_destroy(&m->lock); +} + +void virMutexLock(virMutexPtr m){ + pthread_mutex_lock(&m->lock); +} + +void virMutexUnlock(virMutexPtr m) +{ + pthread_mutex_unlock(&m->lock); +} + + +int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL; + return -1; + } + return 0; +} + +void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock); +} + +void virRWLockWrite(virRWLockPtr m) +{ + pthread_rwlock_wrlock(&m->lock); +} + + +void virRWLockUnlock(virRWLockPtr m) +{ + pthread_rwlock_unlock(&m->lock); +} + +int virCondInit(virCondPtr c) +{ + int ret; + if ((ret = pthread_cond_init(&c->cond, NULL)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondDestroy(virCondPtr c) +{ + int ret; + if ((ret = pthread_cond_destroy(&c->cond)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondWait(virCondPtr c, virMutexPtr m) +{ + int ret; + if ((ret = pthread_cond_wait(&c->cond, &m->lock)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) +{ + int ret; + struct timespec ts; + + ts.tv_sec = whenms / 1000; + ts.tv_nsec = (whenms % 1000) * 1000; + + if ((ret = pthread_cond_timedwait(&c->cond, &m->lock, &ts)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +void virCondSignal(virCondPtr c) +{ + pthread_cond_signal(&c->cond); +} + +void virCondBroadcast(virCondPtr c) +{ + pthread_cond_broadcast(&c->cond); +} + +struct virThreadArgs { + virThreadFunc func; + void *opaque; +}; + +static void *virThreadHelper(void *data) +{ + struct virThreadArgs *args = data; + struct virThreadArgs local = *args; + + /* Free args early, rather than tying it up during the entire thread. */ + VIR_FREE(args); + local.func(local.opaque); + return NULL; +} + +int virThreadCreate(virThreadPtr thread, + bool joinable, + virThreadFunc func, + void *opaque) +{ + struct virThreadArgs *args; + pthread_attr_t attr; + int ret = -1; + int err; + + if ((err = pthread_attr_init(&attr)) != 0) + goto cleanup; + if (VIR_ALLOC_QUIET(args) < 0) { + err = ENOMEM; + goto cleanup; + } + + args->func = func; + args->opaque = opaque; + + if (!joinable) + pthread_attr_setdetachstate(&attr, 1); + + err = pthread_create(&thread->thread, &attr, virThreadHelper, args); + if (err != 0) { + VIR_FREE(args); + goto cleanup; + } + /* New thread owns 'args' in success case, so don't free */ + + ret = 0; +cleanup: + pthread_attr_destroy(&attr); + if (ret < 0) + errno = err; + return ret; +} + +void virThreadSelf(virThreadPtr thread) +{ + thread->thread = pthread_self(); +} + +bool virThreadIsSelf(virThreadPtr thread) +{ + return pthread_equal(pthread_self(), thread->thread) ? true : false; +} + +/* For debugging use only; this result is not guaranteed unique if + * pthread_t is larger than a 64-bit pointer, nor does it always match + * the pthread_self() id on Linux. */ +unsigned long long virThreadSelfID(void) +{ +#if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid) + pid_t tid = syscall(SYS_gettid); + return tid; #else -# error "Either pthreads or Win32 threads are required" + union { + unsigned long long l; + pthread_t t; + } u; + u.t = pthread_self(); + return u.l; #endif +} + +/* For debugging use only; this result is not guaranteed unique if + * pthread_t is larger than a 64-bit pointer, nor does it always match + * the thread id of virThreadSelfID on Linux. */ +unsigned long long virThreadID(virThreadPtr thread) +{ + union { + unsigned long long l; + pthread_t t; + } u; + u.t = thread->thread; + return u.l; +} + +void virThreadJoin(virThreadPtr thread) +{ + pthread_join(thread->thread, NULL); +} + +void virThreadCancel(virThreadPtr thread) +{ + pthread_cancel(thread->thread); +} + +int virThreadLocalInit(virThreadLocalPtr l, + virThreadLocalCleanup c) +{ + int ret; + if ((ret = pthread_key_create(&l->key, c)) != 0) { + errno = ret; + return -1; + } + return 0; +} + +void *virThreadLocalGet(virThreadLocalPtr l) +{ + return pthread_getspecific(l->key); +} + +int virThreadLocalSet(virThreadLocalPtr l, void *val) +{ + int err = pthread_setspecific(l->key, val); + if (err) { + errno = err; + return -1; + } + return 0; +} diff --git a/src/util/virthread.h b/src/util/virthread.h index 7015d60..eba8dc3 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -25,24 +25,57 @@ # include "internal.h" # include "virerror.h" +# include <pthread.h> + typedef struct virMutex virMutex; typedef virMutex *virMutexPtr; +struct virMutex { + pthread_mutex_t lock; +}; + typedef struct virRWLock virRWLock; typedef virRWLock *virRWLockPtr; +struct virRWLock { + pthread_rwlock_t lock; +}; + + typedef struct virCond virCond; typedef virCond *virCondPtr; +struct virCond { + pthread_cond_t cond; +}; + typedef struct virThreadLocal virThreadLocal; typedef virThreadLocal *virThreadLocalPtr; +struct virThreadLocal { + pthread_key_t key; +}; + typedef struct virThread virThread; typedef virThread *virThreadPtr; +struct virThread { + pthread_t thread; +}; + typedef struct virOnceControl virOnceControl; typedef virOnceControl *virOnceControlPtr; +struct virOnceControl { + pthread_once_t once; +}; + + +# define VIR_ONCE_CONTROL_INITIALIZER \ + { \ + .once = PTHREAD_ONCE_INIT \ + } + typedef void (*virOnceFunc)(void); int virThreadInitialize(void) ATTRIBUTE_RETURN_CHECK; @@ -121,14 +154,6 @@ int virThreadLocalInit(virThreadLocalPtr l, void *virThreadLocalGet(virThreadLocalPtr l); int virThreadLocalSet(virThreadLocalPtr l, void*) ATTRIBUTE_RETURN_CHECK; -# ifdef WIN32 -# include "virthreadwin32.h" -# elif defined HAVE_PTHREAD_MUTEXATTR_INIT -# include "virthreadpthread.h" -# else -# error "Either pthreads or Win32 threads are required" -# endif - /** * VIR_ONCE_GLOBAL_INIT: diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c deleted file mode 100644 index 2efb4c1..0000000 --- a/src/util/virthreadpthread.c +++ /dev/null @@ -1,309 +0,0 @@ -/* - * virthreadpthread.c: basic thread synchronization primitives - * - * Copyright (C) 2009-2011, 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>;. - * - */ - -#include <config.h> - -#include <unistd.h> -#include <inttypes.h> -#if HAVE_SYS_SYSCALL_H -# include <sys/syscall.h> -#endif - -#include "viralloc.h" - - -/* Nothing special required for pthreads */ -int virThreadInitialize(void) -{ - return 0; -} - -void virThreadOnExit(void) -{ -} - -int virOnce(virOnceControlPtr once, virOnceFunc init) -{ - return pthread_once(&once->once, init); -} - - -int virMutexInit(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virMutexInitRecursive(virMutexPtr m) -{ - int ret; - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - ret = pthread_mutex_init(&m->lock, &attr); - pthread_mutexattr_destroy(&attr); - if (ret != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virMutexDestroy(virMutexPtr m) -{ - pthread_mutex_destroy(&m->lock); -} - -void virMutexLock(virMutexPtr m){ - pthread_mutex_lock(&m->lock); -} - -void virMutexUnlock(virMutexPtr m) -{ - pthread_mutex_unlock(&m->lock); -} - - -int virRWLockInit(virRWLockPtr m) -{ - if (pthread_rwlock_init(&m->lock, NULL) != 0) { - errno = EINVAL; - return -1; - } - return 0; -} - -void virRWLockDestroy(virRWLockPtr m) -{ - pthread_rwlock_destroy(&m->lock); -} - - -void virRWLockRead(virRWLockPtr m) -{ - pthread_rwlock_rdlock(&m->lock); -} - -void virRWLockWrite(virRWLockPtr m) -{ - pthread_rwlock_wrlock(&m->lock); -} - - -void virRWLockUnlock(virRWLockPtr m) -{ - pthread_rwlock_unlock(&m->lock); -} - -int virCondInit(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_init(&c->cond, NULL)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondDestroy(virCondPtr c) -{ - int ret; - if ((ret = pthread_cond_destroy(&c->cond)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWait(virCondPtr c, virMutexPtr m) -{ - int ret; - if ((ret = pthread_cond_wait(&c->cond, &m->lock)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) -{ - int ret; - struct timespec ts; - - ts.tv_sec = whenms / 1000; - ts.tv_nsec = (whenms % 1000) * 1000; - - if ((ret = pthread_cond_timedwait(&c->cond, &m->lock, &ts)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void virCondSignal(virCondPtr c) -{ - pthread_cond_signal(&c->cond); -} - -void virCondBroadcast(virCondPtr c) -{ - pthread_cond_broadcast(&c->cond); -} - -struct virThreadArgs { - virThreadFunc func; - void *opaque; -}; - -static void *virThreadHelper(void *data) -{ - struct virThreadArgs *args = data; - struct virThreadArgs local = *args; - - /* Free args early, rather than tying it up during the entire thread. */ - VIR_FREE(args); - local.func(local.opaque); - return NULL; -} - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) -{ - struct virThreadArgs *args; - pthread_attr_t attr; - int ret = -1; - int err; - - if ((err = pthread_attr_init(&attr)) != 0) - goto cleanup; - if (VIR_ALLOC_QUIET(args) < 0) { - err = ENOMEM; - goto cleanup; - } - - args->func = func; - args->opaque = opaque; - - if (!joinable) - pthread_attr_setdetachstate(&attr, 1); - - err = pthread_create(&thread->thread, &attr, virThreadHelper, args); - if (err != 0) { - VIR_FREE(args); - goto cleanup; - } - /* New thread owns 'args' in success case, so don't free */ - - ret = 0; -cleanup: - pthread_attr_destroy(&attr); - if (ret < 0) - errno = err; - return ret; -} - -void virThreadSelf(virThreadPtr thread) -{ - thread->thread = pthread_self(); -} - -bool virThreadIsSelf(virThreadPtr thread) -{ - return pthread_equal(pthread_self(), thread->thread) ? true : false; -} - -/* For debugging use only; this result is not guaranteed unique if - * pthread_t is larger than a 64-bit pointer, nor does it always match - * the pthread_self() id on Linux. */ -unsigned long long virThreadSelfID(void) -{ -#if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid) - pid_t tid = syscall(SYS_gettid); - return tid; -#else - union { - unsigned long long l; - pthread_t t; - } u; - u.t = pthread_self(); - return u.l; -#endif -} - -/* For debugging use only; this result is not guaranteed unique if - * pthread_t is larger than a 64-bit pointer, nor does it always match - * the thread id of virThreadSelfID on Linux. */ -unsigned long long virThreadID(virThreadPtr thread) -{ - union { - unsigned long long l; - pthread_t t; - } u; - u.t = thread->thread; - return u.l; -} - -void virThreadJoin(virThreadPtr thread) -{ - pthread_join(thread->thread, NULL); -} - -void virThreadCancel(virThreadPtr thread) -{ - pthread_cancel(thread->thread); -} - -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) -{ - int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { - errno = ret; - return -1; - } - return 0; -} - -void *virThreadLocalGet(virThreadLocalPtr l) -{ - return pthread_getspecific(l->key); -} - -int virThreadLocalSet(virThreadLocalPtr l, void *val) -{ - int err = pthread_setspecific(l->key, val); - if (err) { - errno = err; - return -1; - } - return 0; -} diff --git a/src/util/virthreadpthread.h b/src/util/virthreadpthread.h deleted file mode 100644 index cb607d0..0000000 --- a/src/util/virthreadpthread.h +++ /dev/null @@ -1,53 +0,0 @@ -/* - * virthreadpthread.c: basic thread synchronization primitives - * - * Copyright (C) 2009, 2011 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>;. - * - */ - -#include "internal.h" - -#include <pthread.h> - -struct virMutex { - pthread_mutex_t lock; -}; - -struct virRWLock { - pthread_rwlock_t lock; -}; - -struct virCond { - pthread_cond_t cond; -}; - -struct virThread { - pthread_t thread; -}; - -struct virThreadLocal { - pthread_key_t key; -}; - -struct virOnceControl { - pthread_once_t once; -}; - -#define VIR_ONCE_CONTROL_INITIALIZER \ -{ \ - .once = PTHREAD_ONCE_INIT \ -} diff --git a/src/util/virthreadwin32.c b/src/util/virthreadwin32.c deleted file mode 100644 index a9e2353..0000000 --- a/src/util/virthreadwin32.c +++ /dev/null @@ -1,415 +0,0 @@ -/* - * virthreadwin32.c: basic thread synchronization primitives - * - * Copyright (C) 2009-2011, 2013 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>;. - * - */ - -#include <config.h> - -#include <process.h> - -#include "viralloc.h" - -#define VIR_FROM_THIS VIR_FROM_NONE - -struct virThreadLocalData { - DWORD key; - virThreadLocalCleanup cleanup; -}; -typedef struct virThreadLocalData virThreadLocalData; -typedef virThreadLocalData *virThreadLocalDataPtr; - -virMutex virThreadLocalLock; -size_t virThreadLocalCount = 0; -virThreadLocalDataPtr virThreadLocalList = NULL; -DWORD selfkey; - -virThreadLocal virCondEvent; - -void virCondEventCleanup(void *data); - -int virThreadInitialize(void) -{ - if (virMutexInit(&virThreadLocalLock) < 0) - return -1; - if (virThreadLocalInit(&virCondEvent, virCondEventCleanup) < 0) - return -1; - if ((selfkey = TlsAlloc()) == TLS_OUT_OF_INDEXES) - return -1; - return 0; -} - -void virThreadOnExit(void) -{ - size_t i; - virMutexLock(&virThreadLocalLock); - for (i = 0; i < virThreadLocalCount; i++) { - if (virThreadLocalList[i].cleanup) { - void *data = TlsGetValue(virThreadLocalList[i].key); - if (data) { - TlsSetValue(virThreadLocalList[i].key, NULL); - - (virThreadLocalList[i].cleanup)(data); - } - } - } - virMutexUnlock(&virThreadLocalLock); -} - -int virOnce(virOnceControlPtr once, virOnceFunc func) -{ - if (!once->complete) { - if (InterlockedIncrement(&once->init) == 1) { - /* We're the first thread. */ - func(); - once->complete = 1; - } else { - /* We're a later thread. Decrement the init counter back - * to avoid overflow, then yield until the first thread - * marks that the function is complete. It is rare that - * multiple threads will be waiting here, and since each - * thread is yielding except the first, we should get out - * soon enough. */ - InterlockedDecrement(&once->init); - while (!once->complete) - Sleep(0); - } - } - return 0; -} - -int virMutexInit(virMutexPtr m) -{ - return virMutexInitRecursive(m); -} - -int virMutexInitRecursive(virMutexPtr m) -{ - if (!(m->lock = CreateMutex(NULL, FALSE, NULL))) { - errno = ESRCH; - return -1; - } - return 0; -} - -void virMutexDestroy(virMutexPtr m) -{ - CloseHandle(m->lock); -} - -void virMutexLock(virMutexPtr m) -{ - WaitForSingleObject(m->lock, INFINITE); -} - -void virMutexUnlock(virMutexPtr m) -{ - ReleaseMutex(m->lock); -} - - -int virRWLockInit(virRWLockPtr m ATTRIBUTE_UNUSED) -{ - errno = ENOSYS; - return -1; -} - -void virRWLockDestroy(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - - -void virRWLockRead(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - -void virRWLockWrite(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - - -void virRWLockUnlock(virRWLockPtr m ATTRIBUTE_UNUSED) -{} - -int virCondInit(virCondPtr c) -{ - c->waiters = NULL; - if (virMutexInit(&c->lock) < 0) - return -1; - return 0; -} - -int virCondDestroy(virCondPtr c) -{ - if (c->waiters) { - errno = EINVAL; - return -1; - } - virMutexDestroy(&c->lock); - return 0; -} - -void virCondEventCleanup(void *data) -{ - HANDLE event = data; - CloseHandle(event); -} - -int virCondWait(virCondPtr c, virMutexPtr m) -{ - HANDLE event = virThreadLocalGet(&virCondEvent); - - if (!event) { - event = CreateEvent(0, FALSE, FALSE, NULL); - if (!event) { - return -1; - } - if (virThreadLocalSet(&virCondEvent, event) < 0) { - CloseHandle(event); - return -1; - } - } - - virMutexLock(&c->lock); - - if (VIR_REALLOC_N(c->waiters, c->nwaiters + 1) < 0) { - virMutexUnlock(&c->lock); - return -1; - } - c->waiters[c->nwaiters] = event; - c->nwaiters++; - - virMutexUnlock(&c->lock); - - virMutexUnlock(m); - - if (WaitForSingleObject(event, INFINITE) == WAIT_FAILED) { - virMutexLock(m); - errno = EINVAL; - return -1; - } - - virMutexLock(m); - return 0; -} - -int virCondWaitUntil(virCondPtr c ATTRIBUTE_UNUSED, - virMutexPtr m ATTRIBUTE_UNUSED, - unsigned long long whenms ATTRIBUTE_UNUSED) -{ - /* FIXME: this function is currently only used by the QEMU driver that - * is not compiled on Windows, so it's okay for now to just - * miss an implementation */ - return -1; -} - -void virCondSignal(virCondPtr c) -{ - virMutexLock(&c->lock); - - if (c->nwaiters) { - HANDLE event = c->waiters[0]; - if (c->nwaiters > 1) - memmove(c->waiters, - c->waiters + 1, - sizeof(c->waiters[0]) * (c->nwaiters-1)); - if (VIR_REALLOC_N(c->waiters, c->nwaiters - 1) < 0) { - ; - } - c->nwaiters--; - SetEvent(event); - } - - virMutexUnlock(&c->lock); -} - -void virCondBroadcast(virCondPtr c) -{ - virMutexLock(&c->lock); - - if (c->nwaiters) { - size_t i; - for (i = 0; i < c->nwaiters; i++) { - HANDLE event = c->waiters[i]; - SetEvent(event); - } - VIR_FREE(c->waiters); - c->nwaiters = 0; - } - - virMutexUnlock(&c->lock); -} - - -struct virThreadArgs { - virThreadFunc func; - void *opaque; -}; - -static void virThreadHelperDaemon(void *data) -{ - struct virThreadArgs *args = data; - virThread self; - HANDLE handle = GetCurrentThread(); - HANDLE process = GetCurrentProcess(); - - self.joinable = false; - DuplicateHandle(process, handle, process, - &self.thread, 0, FALSE, - DUPLICATE_SAME_ACCESS); - TlsSetValue(selfkey, &self); - - args->func(args->opaque); - - TlsSetValue(selfkey, NULL); - CloseHandle(self.thread); - - VIR_FREE(args); -} - -static unsigned int __stdcall virThreadHelperJoinable(void *data) -{ - struct virThreadArgs *args = data; - virThread self; - HANDLE handle = GetCurrentThread(); - HANDLE process = GetCurrentProcess(); - - self.joinable = true; - DuplicateHandle(process, handle, process, - &self.thread, 0, FALSE, - DUPLICATE_SAME_ACCESS); - TlsSetValue(selfkey, &self); - - args->func(args->opaque); - - TlsSetValue(selfkey, NULL); - CloseHandle(self.thread); - - VIR_FREE(args); - return 0; -} - -int virThreadCreate(virThreadPtr thread, - bool joinable, - virThreadFunc func, - void *opaque) -{ - struct virThreadArgs *args; - uintptr_t ret; - - if (VIR_ALLOC(args) < 0) - return -1; - - args->func = func; - args->opaque = opaque; - - thread->joinable = joinable; - if (joinable) { - ret = _beginthreadex(NULL, 0, - virThreadHelperJoinable, - args, 0, NULL); - if (ret == 0) - return -1; - } else { - ret = _beginthread(virThreadHelperDaemon, - 0, args); - if (ret == -1L) - return -1; - } - - thread->thread = (HANDLE)ret; - - return 0; -} - -void virThreadSelf(virThreadPtr thread) -{ - virThreadPtr self = TlsGetValue(selfkey); - - if (self == NULL) { - /* called on a thread not created by virThreadCreate, e.g. the main thread */ - thread->thread = 0; - thread->joinable = false; - } else { - thread->thread = self->thread; - thread->joinable = self->joinable; - } -} - -bool virThreadIsSelf(virThreadPtr thread) -{ - virThread self; - virThreadSelf(&self); - return self.thread == thread->thread ? true : false; -} - -/* For debugging use only; see comments in virthreadpthread.c. */ -unsigned long long virThreadSelfID(void) -{ - return GetCurrentThreadId(); -} - -/* For debugging use only; see comments in virthreadpthread.c. */ -unsigned long long virThreadID(virThreadPtr thread) -{ - return (intptr_t)thread->thread; -} - - -void virThreadJoin(virThreadPtr thread) -{ - if (thread->joinable) { - WaitForSingleObject(thread->thread, INFINITE); - CloseHandle(thread->thread); - thread->thread = 0; - thread->joinable = false; - } -} - -void virThreadCancel(virThreadPtr thread ATTRIBUTE_UNUSED) -{} - -int virThreadLocalInit(virThreadLocalPtr l, - virThreadLocalCleanup c) -{ - if ((l->key = TlsAlloc()) == TLS_OUT_OF_INDEXES) { - errno = ESRCH; - return -1; - } - TlsSetValue(l->key, NULL); - - if (c) { - virMutexLock(&virThreadLocalLock); - if (VIR_REALLOC_N(virThreadLocalList, - virThreadLocalCount + 1) < 0) - return -1; - virThreadLocalList[virThreadLocalCount].key = l->key; - virThreadLocalList[virThreadLocalCount].cleanup = c; - virThreadLocalCount++; - virMutexUnlock(&virThreadLocalLock); - } - - return 0; -} - -void *virThreadLocalGet(virThreadLocalPtr l) -{ - return TlsGetValue(l->key); -} - -int virThreadLocalSet(virThreadLocalPtr l, void *val) -{ - return TlsSetValue(l->key, val) == 0 ? -1 : 0; -} diff --git a/src/util/virthreadwin32.h b/src/util/virthreadwin32.h deleted file mode 100644 index 31d9444..0000000 --- a/src/util/virthreadwin32.h +++ /dev/null @@ -1,57 +0,0 @@ -/* - * virthreadwin32.h basic thread synchronization primitives - * - * Copyright (C) 2009, 2011-2012 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>;. - * - */ - -#include "internal.h" - -#ifdef HAVE_WINSOCK2_H -# include <winsock2.h> -#endif -#include <windows.h> - -struct virMutex { - HANDLE lock; -}; - -struct virRWLock { - bool ignored; -}; - -struct virCond { - virMutex lock; - size_t nwaiters; - HANDLE *waiters; -}; - -struct virThread { - HANDLE thread; - bool joinable; -}; - -struct virThreadLocal { - DWORD key; -}; - -struct virOnceControl { - volatile long init; /* 0 at startup, > 0 if init has started */ - volatile long complete; /* 0 until first thread completes callback */ -}; - -#define VIR_ONCE_CONTROL_INITIALIZER { 0, 0 } -- 1.8.4.2

The nwfilter conf update mutex previously serialized updates to the internal data structures for firewall rules, and updates to the firewall itself. Since the former is going to be turned into a read/write lock instead of a mutex, a new lock is required to serialize access to the firewall itself. With this new lock, the lock ordering rules will be for virNWFilter{Define,Undefine} 1. nwfilter driver lock 2. nwfilter update lock 3. virt driver lock 4. domain object lock 5. gentech driver lock and VM start 1. nwfilter update lock 2. virt driver lock 3. domain object lock 4. gentech driver lock Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/nwfilter/nwfilter_driver.c | 4 +++- src/nwfilter/nwfilter_gentech_driver.c | 19 +++++++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d21dd82..112e8cb 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -200,7 +200,8 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterDHCPSnoopInit() < 0) goto err_exit_learnshutdown; - virNWFilterTechDriversInit(privileged); + if (virNWFilterTechDriversInit(privileged) < 0) + goto err_dhcpsnoop_shutdown; if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB, driverState) < 0) @@ -251,6 +252,7 @@ error: err_techdrivers_shutdown: virNWFilterTechDriversShutdown(); +err_dhcpsnoop_shutdown: virNWFilterDHCPSnoopShutdown(); err_exit_learnshutdown: virNWFilterLearnShutdown(); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..d500963 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -55,15 +55,21 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = { NULL }; +/* Serializes execution of iptables/ip6tables/ebtables calls */ +static virMutex updateMutex; -void virNWFilterTechDriversInit(bool privileged) { +int virNWFilterTechDriversInit(bool privileged) { size_t i = 0; VIR_DEBUG("Initializing NWFilter technology drivers"); + if (virMutexInitRecursive(&updateMutex) < 0) + return -1; + while (filter_tech_drivers[i]) { if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)) filter_tech_drivers[i]->init(privileged); i++; } + return 0; } @@ -74,6 +80,7 @@ void virNWFilterTechDriversShutdown(void) { filter_tech_drivers[i]->shutdown(); i++; } + virMutexDestroy(&updateMutex); } @@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int rc; virNWFilterLockFilterUpdates(); + virMutexLock(&updateMutex); /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed @@ -965,6 +973,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, cleanup: virNWFilterUnlockFilterUpdates(); + virMutexUnlock(&updateMutex); return rc; } @@ -985,6 +994,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, bool foundNewFilter = false; virNWFilterLockFilterUpdates(); + virMutexLock(&updateMutex); rc = __virNWFilterInstantiateFilter(driver, vmuuid, @@ -1010,6 +1020,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, } virNWFilterUnlockFilterUpdates(); + virMutexUnlock(&updateMutex); return rc; } @@ -1133,7 +1144,11 @@ _virNWFilterTeardownFilter(const char *ifname) int virNWFilterTeardownFilter(const virDomainNetDef *net) { - return _virNWFilterTeardownFilter(net->ifname); + int ret; + virMutexLock(&updateMutex); + ret = _virNWFilterTeardownFilter(net->ifname); + virMutexUnlock(&updateMutex); + return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index f4789e1..d72e040 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -31,7 +31,7 @@ virNWFilterTechDriverPtr virNWFilterTechDriverForName(const char *name); int virNWFilterRuleInstAddData(virNWFilterRuleInstPtr res, void *data); -void virNWFilterTechDriversInit(bool privileged); +int virNWFilterTechDriversInit(bool privileged); void virNWFilterTechDriversShutdown(void); enum instCase { -- 1.8.4.2

On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote: It wasn't entirely clear to me either, but I am right in believing that it isn't safe to have multiple threads all creating iptables rules for different VMs at the same time, aren't I ? The callgraph showed the three entry points into this area of code look like: virNWFilterInstantiateFilterLate virNWFilterInstantiateFilter virNWFilterTeardownFilter | | | +-------------------------|-----+ | | +------------+ | | | | | | V V V V _virNWFilterInstantiateFilter _virNWFilterTeardownFilter Looking at the code I don't see how it is safe to allow teardown to happen in parallel with instantiate. The teardown code could confuse and conflict with the instantiate code causing it to fail I believe. In particular a VM could exit causing QEMU to request teardown, while the DHCP snoop thread is in the middle of re-creating the iptables ruleset for a VM, so we surely require serialization of modifications to iptables. I could, push the locking down one level, but it wouldn't change the level of serialization, and the lock calls are clearer at the level of the code I have them I believe. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 01/27/2014 12:18 PM, Daniel P. Berrange wrote: Insert: 3. nwfilter callback drivers lock This is then used in this order also by nwfilterStateReload stray newline here Modulo the above stray line and then comment in the introductory text, I think this patch is good. I am just a bit concerned about 3/4. Stefan