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@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",
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list