[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]