Hi Eric,
On Thu, May 02, 2013 at 03:58:23PM -0600, Eric Blake wrote:
> POSIX says pthread_t is opaque. We can't guarantee if it is scaler
> or a pointer, nor what size it is; and BSD differs from Linux.
> We've also had reports of gcc complaining on attempts to cast it,
> if we use a cast to the wrong type (for example, pointers have to be
> cast to void* or intptr_t before being narrowed; while casting a
> function return of pthread_t to void* triggers another warning).
>
> Give up on casts, and use unions to get at decent bits instead. And
> rather than futz around with figuring which 32 bits of a potentially
> 64-bit pointer are most likely to be unique, convert the rest of
> the code base to use 64-bit values when using a debug id.
>
> Based on a report by Guido Günther against kFreeBSD, but with a
> fix that doesn't regress commit 4d970fd29 for FreeBSD.
Works on kFreeBSD here, thanks.
-- Guido
>
> * src/util/virthreadpthread.c (virThreadSelfID, virThreadID): Use
> union to get at a decent bit representation of thread_t bits.
> * src/util/virthread.h (virThreadSelfID, virThreadID): Alter
> signature.
> * src/util/virthreadwin32.c (virThreadSelfID, virThreadID):
> Likewise.
> * src/qemu/qemu_domain.h (qemuDomainJobObj): Alter type of owner.
> * src/qemu/qemu_domain.c (qemuDomainObjTransferJob)
> (qemuDomainObjSetJobPhase, qemuDomainObjReleaseAsyncJob)
> (qemuDomainObjBeginNestedJob, qemuDomainObjBeginJobInternal): Fix
> clients.
> * src/util/virlog.c (virLogFormatString): Likewise.
> * src/util/vireventpoll.c (virEventPollInterruptLocked):
> Likewise.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>
> This one needs a review.
>
> I've tested this on Fedora 18 and FreeBSD 8.2; I also ran it through
> the mingw64 cross-compiler on Fedora (thanks to autobuild.sh), without
> grief. But I don't have a kFreeBSD system handy, which was the
> original source of the compilation complaint.
>
> src/qemu/qemu_domain.c | 12 ++++++------
> src/qemu/qemu_domain.h | 4 ++--
> src/util/vireventpoll.c | 4 ++--
> src/util/virlog.c | 6 +++---
> src/util/virthread.h | 6 +++---
> src/util/virthreadpthread.c | 33 ++++++++++++++++++++++-----------
> src/util/virthreadwin32.c | 12 ++++++------
> 7 files changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 47edfba..bfc57e5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -187,7 +187,7 @@ qemuDomainObjTransferJob(virDomainObjPtr obj)
> {
> qemuDomainObjPrivatePtr priv = obj->privateData;
>
> - VIR_DEBUG("Changing job owner from %d to %d",
> + VIR_DEBUG("Changing job owner from %lld to %lld",
> priv->job.owner, virThreadSelfID());
> priv->job.owner = virThreadSelfID();
> }
> @@ -846,7 +846,7 @@ qemuDomainObjSetJobPhase(virQEMUDriverPtr driver,
> int phase)
> {
> qemuDomainObjPrivatePtr priv = obj->privateData;
> - int me = virThreadSelfID();
> + unsigned long long int me = virThreadSelfID();
>
> if (!priv->job.asyncJob)
> return;
> @@ -856,7 +856,7 @@ qemuDomainObjSetJobPhase(virQEMUDriverPtr driver,
> qemuDomainAsyncJobPhaseToString(priv->job.asyncJob, phase));
>
> if (priv->job.asyncOwner && me != priv->job.asyncOwner) {
> - VIR_WARN("'%s' async job is owned by thread %d",
> + VIR_WARN("'%s' async job is owned by thread %lld",
> qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> priv->job.asyncOwner);
> }
> @@ -898,7 +898,7 @@ qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj)
> qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
>
> if (priv->job.asyncOwner != virThreadSelfID()) {
> - VIR_WARN("'%s' async job is owned by thread %d",
> + VIR_WARN("'%s' async job is owned by thread %lld",
> qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
> priv->job.asyncOwner);
> }
> @@ -992,7 +992,7 @@ retry:
>
> error:
> VIR_WARN("Cannot start job (%s, %s) for domain %s;"
> - " current job is (%s, %s) owned by (%d, %d)",
> + " current job is (%s, %s) owned by (%lld, %lld)",
> qemuDomainJobTypeToString(job),
> qemuDomainAsyncJobTypeToString(asyncJob),
> obj->def->name,
> @@ -1056,7 +1056,7 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
> }
>
> if (priv->job.asyncOwner != virThreadSelfID()) {
> - VIR_WARN("This thread doesn't seem to be the async job owner: %d",
> + VIR_WARN("This thread doesn't seem to be the async job owner: %lld",
> priv->job.asyncOwner);
> }
>
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index da04377..7746ba9 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -102,11 +102,11 @@ VIR_ENUM_DECL(qemuDomainAsyncJob)
> struct qemuDomainJobObj {
> virCond cond; /* Use to coordinate jobs */
> enum qemuDomainJob active; /* Currently running job */
> - int owner; /* Thread which set current job */
> + unsigned long long int owner; /* Thread id which set current job */
>
> virCond asyncCond; /* Use to coordinate with async jobs */
> enum qemuDomainAsyncJob asyncJob; /* Currently active async job */
> - int asyncOwner; /* Thread which set current async job */
> + unsigned long long int asyncOwner; /* Thread which set current async job */
> int phase; /* Job phase (mainly for migrations) */
> unsigned long long mask; /* Jobs allowed during async job */
> unsigned long long start; /* When the async job started */
> diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
> index 5c6d31b..19add73 100644
> --- a/src/util/vireventpoll.c
> +++ b/src/util/vireventpoll.c
> @@ -1,7 +1,7 @@
> /*
> * vireventpoll.c: Poll based event loop for monitoring file handles
> *
> - * Copyright (C) 2007, 2010-2012 Red Hat, Inc.
> + * Copyright (C) 2007, 2010-2013 Red Hat, Inc.
> * Copyright (C) 2007 Daniel P. Berrange
> *
> * This library is free software; you can redistribute it and/or
> @@ -708,7 +708,7 @@ static int virEventPollInterruptLocked(void)
>
> if (!eventLoop.running ||
> virThreadIsSelf(&eventLoop.leader)) {
> - VIR_DEBUG("Skip interrupt, %d %d", eventLoop.running,
> + VIR_DEBUG("Skip interrupt, %d %lld", eventLoop.running,
> virThreadID(&eventLoop.leader));
> return 0;
> }
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index bd47b38..b2e6458 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1,7 +1,7 @@
> /*
> * virlog.c: internal logging and debugging
> *
> - * Copyright (C) 2008, 2010-2012 Red Hat, Inc.
> + * Copyright (C) 2008, 2010-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
> @@ -708,11 +708,11 @@ virLogFormatString(char **msg,
> * to just grep for it to find the right place.
> */
> if ((funcname != NULL)) {
> - ret = virAsprintf(msg, "%d: %s : %s:%d : %s\n",
> + ret = virAsprintf(msg, "%lld: %s : %s:%d : %s\n",
> virThreadSelfID(), virLogPriorityString(priority),
> funcname, linenr, str);
> } else {
> - ret = virAsprintf(msg, "%d: %s : %s\n",
> + ret = virAsprintf(msg, "%lld: %s : %s\n",
> virThreadSelfID(), virLogPriorityString(priority),
> str);
> }
> diff --git a/src/util/virthread.h b/src/util/virthread.h
> index ca1306a..447fe84 100644
> --- a/src/util/virthread.h
> +++ b/src/util/virthread.h
> @@ -1,7 +1,7 @@
> /*
> * virthread.h: basic thread synchronization primitives
> *
> - * Copyright (C) 2009-2011 Red Hat, Inc.
> + * 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
> @@ -63,8 +63,8 @@ void virThreadCancel(virThreadPtr thread);
> * guaranteed to give unique values for distinct threads on all
> * architectures, nor are the two functions guaranteed to give the same
> * value for the same thread. */
> -int virThreadSelfID(void);
> -int virThreadID(virThreadPtr thread);
> +unsigned long long int virThreadSelfID(void);
> +unsigned long long int virThreadID(virThreadPtr thread);
>
> /* Static initialization of mutexes is not possible, so we instead
> * provide for guaranteed one-time initialization via a callback
> diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c
> index b42b333..20fa539 100644
> --- a/src/util/virthreadpthread.c
> +++ b/src/util/virthreadpthread.c
> @@ -1,7 +1,7 @@
> /*
> * virthreadpthread.c: basic thread synchronization primitives
> *
> - * Copyright (C) 2009-2011 Red Hat, Inc.
> + * 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
> @@ -210,25 +210,36 @@ bool virThreadIsSelf(virThreadPtr thread)
> return pthread_equal(pthread_self(), thread->thread) ? true : false;
> }
>
> -/* For debugging use only; this result is not guaranteed unique on BSD
> - * systems when pthread_t is a 64-bit pointer. */
> -int virThreadSelfID(void)
> +/* 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 int virThreadSelfID(void)
> {
> #if defined(HAVE_SYS_SYSCALL_H) && defined(SYS_gettid)
> pid_t tid;
> tid = syscall(SYS_gettid);
> - return (int)tid;
> + return tid;
> #else
> - return (int)(intptr_t)(void *)pthread_self();
> + union {
> + unsigned long long int i;
> + pthread_t t;
> + } u;
> + u.t = pthread_self();
> + return u.i;
> #endif
> }
>
> -/* For debugging use only; this result is not guaranteed unique on BSD
> - * systems when pthread_t is a 64-bit pointer, nor does it match the
> - * thread id of virThreadSelfID on Linux. */
> -int virThreadID(virThreadPtr thread)
> +/* 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 int virThreadID(virThreadPtr thread)
> {
> - return (int)(uintptr_t)thread->thread;
> + union {
> + unsigned long long int i;
> + pthread_t t;
> + } u;
> + u.t = thread->thread;
> + return u.i;
> }
>
> void virThreadJoin(virThreadPtr thread)
> diff --git a/src/util/virthreadwin32.c b/src/util/virthreadwin32.c
> index 4543ad8..10837d2 100644
> --- a/src/util/virthreadwin32.c
> +++ b/src/util/virthreadwin32.c
> @@ -1,7 +1,7 @@
> /*
> * virthreadwin32.c: basic thread synchronization primitives
> *
> - * Copyright (C) 2009-2011 Red Hat, Inc.
> + * 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
> @@ -334,14 +334,14 @@ bool virThreadIsSelf(virThreadPtr thread)
> return self.thread == thread->thread ? true : false;
> }
>
> -/* For debugging use only; see comments in threads-pthread.c. */
> -int virThreadSelfID(void)
> +/* For debugging use only; see comments in virthreadpthread.c. */
> +unsigned long long int virThreadSelfID(void)
> {
> - return (int)GetCurrentThreadId();
> + return GetCurrentThreadId();
> }
>
> -/* For debugging use only; see comments in threads-pthread.c. */
> -int virThreadID(virThreadPtr thread)
> +/* For debugging use only; see comments in virthreadpthread.c. */
> +unsigned long long int virThreadID(virThreadPtr thread)
> {
> return (intptr_t)thread->thread;
> }
> --
> 1.8.1.4
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list