Daniel P. Berrange wrote:

On Thu, May 02, 2013 at 02:05:19PM +0200, Guido Günther wrote:

...

This fixes the build on kFreeBSD that otherwise fails with:

util/virthreadpthread.c: In function 'virThreadSelfID': util/virthreadpthread.c:222:27: error: cast from function call of type 'pthread_t' to non-matching type 'void *' [-Werror=bad-function-cast] --- I'm unsure why we'd need the cast to (void*) first. -- Guido

src/util/virthreadpthread.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virthreadpthread.c b/src/util/virthreadpthread.c index b42b333..8d5e197 100644 --- a/src/util/virthreadpthread.c +++ b/src/util/virthreadpthread.c @@ -219,7 +219,7 @@ int virThreadSelfID(void) tid = syscall(SYS_gettid); return (int)tid; #else - return (int)(intptr_t)(void *)pthread_self(); + return (int)(intptr_t)pthread_self(); #endif

This was introduced in

commit 4d970fd2938a0558444cddb4f5f5e63b910b5527 Author: Eric Blake <eblake@redhat.com> Date: Fri Nov 4 16:32:30 2011 -0600

build: silence compiler warning on BSD

Building on 64-bit FreeBSD 8.2 complained about a cast between a pointer and a smaller integer. Going through an intermediate cast shuts up the compiler.

* src/util/threads-pthread.c (virThreadSelfID): Silence a warning.

diff --git a/src/util/threads-pthread.c b/src/util/threads-pthread.c index 82ce5c6..5b8fd5b 100644 --- a/src/util/threads-pthread.c +++ b/src/util/threads-pthread.c @@ -216,7 +216,7 @@ int virThreadSelfID(void) tid = syscall(SYS_gettid); return (int)tid; #else - return (int)(void *)pthread_self(); + return (int)(intptr_t)(void *)pthread_self();

Before removing the 'void *' we need to check it won't break FreeBSD again.

This change indeed brings a warning on FreeBSD: In file included from util/virthread.c:31: util/virthreadpthread.c: In function 'virThreadSelfID': util/virthreadpthread.c:222: warning: cast from function call of type 'pthread_t' to non-matching type 'long int' [-Wbad-function-cast] Roman Bogorodskiy

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@redhat.com> ---

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

On 05/03/2013 02:58 AM, Daniel P. Berrange wrote:

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

...

- VIR_DEBUG("Changing job owner from %d to %d", + VIR_DEBUG("Changing job owner from %lld to %lld",

s/lld/llu/ since you declared it unsigned

Sure, easy enough to do.

...

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();

s/int//; 'unsigned long long' is verbose enough already without adding a redundent 'int' suffix on it too. Same throughout this patch.

Consider it done :) Will push shortly with those fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org