Re: [Qemu-devel] [PATCH 1/4] fifolock: create rfifolock_is_locked helper

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote:
> +int  rfifolock_is_locked(RFifoLock *r);

Please use bool instead of int.

> diff --git a/util/rfifolock.c b/util/rfifolock.c
> index afbf748..8ac58cb 100644
> --- a/util/rfifolock.c
> +++ b/util/rfifolock.c
> @@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r)
>      /* Take a ticket */
>      unsigned int ticket = r->tail++;
>  
> -    if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) {
> +    if (r->nesting > 0 && rfifolock_is_locked(r)) {
>          r->tail--; /* put ticket back, we're nesting */
>      } else {
>          while (ticket != r->head) {
> @@ -69,10 +69,15 @@ void rfifolock_unlock(RFifoLock *r)
>  {
>      qemu_mutex_lock(&r->lock);
>      assert(r->nesting > 0);
> -    assert(qemu_thread_is_self(&r->owner_thread));
> +    assert(rfifolock_is_locked(r));
>      if (--r->nesting == 0) {
>          r->head++;
>          qemu_cond_broadcast(&r->cond);
>      }
>      qemu_mutex_unlock(&r->lock);
>  }
> +
> +int rfifolock_is_locked(RFifoLock *r)
> +{
> +    return qemu_thread_is_self(&r->owner_thread);
> +}

The function name confused me since "does the current thread hold the
lock?" != "does anyone currently hold the lock?".

I suggest:

bool rfifolock_held_by_current_thread(RFifoLock *r) {
    return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread);
}

Then the r->nesting > 0 testing can also be dropped by callers, which is
good since rfifolock_is_locked() does not return a meaningful result
when r->nesting == 0.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]