7:05 a.m.
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
}
--
1.7.10.4
7:10 a.m.
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(a)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.
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 :|
8 a.m.
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(a)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
8:54 a.m.
On 05/02/2013 06:05 AM, 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.
EVERYTHING should be castable to void*. That's lame that your compiler
is refusing to cast pthread_t.
I'm thinking that the solution will require writing a union, assign to
the pthread_t member of the union, and read out the int member, as the
only way we're going to shut up compilers. We KNOW that casting
pthread_t isn't portable; but all we really want is a few bits that are
hopefully likely enough to be unique to make the use of virThreadSelfID
useful in debug messages. We aren't relying on the conversion being a
1:1 mapping nor on trying to reverse the mapping.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:57 a.m.
On Thu, May 02, 2013 at 07:54:47AM -0600, Eric Blake wrote:
On 05/02/2013 06:05 AM, 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.
EVERYTHING should be castable to void*. That's lame that your compiler
is refusing to cast pthread_t.
I'm thinking that the solution will require writing a union, assign to
the pthread_t member of the union, and read out the int member, as the
only way we're going to shut up compilers. We KNOW that casting
pthread_t isn't portable; but all we really want is a few bits that are
hopefully likely enough to be unique to make the use of virThreadSelfID
useful in debug messages. We aren't relying on the conversion being a
1:1 mapping nor on trying to reverse the mapping.
Yeah, using a union here sounds like a good idea.
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 :|
9:42 a.m.
At Thu, 02 May 2013 07:54:47 -0600,
Eric Blake wrote:
[1 <multipart/signed (7bit)>]
[1.1 <text/plain; UTF-8 (quoted-printable)>]
On 05/02/2013 06:05 AM, 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.
EVERYTHING should be castable to void*. That's lame that your compiler
is refusing to cast pthread_t.
I'm thinking that the solution will require writing a union, assign to
the pthread_t member of the union, and read out the int member, as the
only way we're going to shut up compilers. We KNOW that casting
pthread_t isn't portable; but all we really want is a few bits that are
hopefully likely enough to be unique to make the use of virThreadSelfID
useful in debug messages. We aren't relying on the conversion being a
1:1 mapping nor on trying to reverse the mapping.
Since the warning only ever refers to directly casting a function
return value, wouldn't it suffice to introduce a (properly typed)
variable first assigning the function result?
pthread_t self = pthread_self();
return (int)(intptr_t)self;
Just my 2¢
--
AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany
Phone: +49 341 265 310 19
Web:<http://www.av-test.org>
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
9:49 a.m.
On 05/02/2013 08:42 AM, Claudio Bley wrote:
Since the warning only ever refers to directly casting a function
return value, wouldn't it suffice to introduce a (properly typed)
variable first assigning the function result?
pthread_t self = pthread_self();
return (int)(intptr_t)self;
You _still_ need a (void*) in the mix; pthread_t and intptr_t are not
necessarily compatible, but everything should be compatible with void*.
But a union is still nicer than three casts in a row.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:58 p.m.
New subject: [libvirt] [PATCH] build: avoid non-portable cast of pthread_t
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.
* 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(a)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
1:19 a.m.
New subject: [libvirt] [PATCH] build: avoid non-portable cast of pthread_t
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(a)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
3:58 a.m.
New subject: [libvirt] [PATCH] build: avoid non-portable cast of pthread_t
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.
* 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(a)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",
s/lld/llu/ since you declared it unsigned
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.
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",
s/lld/llu/
@@ -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",
s/lld/llu/
@@ -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)",
s/lld/llu/g
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",
s/lld/llu/
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 */
s/int//;
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 */
s/int//;
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
@@ -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,
s/lld/llu/
etc, etc.
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 :|
7:05 a.m.
New subject: [libvirt] [PATCH] build: avoid non-portable cast of pthread_t
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
4222
days inactive
4223
days old
10 comments
5 participants
participants (5)
-
Claudio Bley -
Daniel P. Berrange -
Eric Blake -
Guido Günther -
Roman Bogorodskiy