[PATCH] lock reference count improvements

[Michael Adam]

On 2018-09-18 at 23:09 +0530, Anoop C S via samba-technical wrote:
> Hi,
> 
> Attached are two patches which improves an assertion check and DEBUG statements around lock
> reference count code inside source3/locking.

Good catch, is there a bug for this? Should we have one?

> Reviews are appreciated.

See below:

> Thanks,
> Anoop C S

> From 3e4645f14f3963d5df6b85e5b25514555641d460 Mon Sep 17 00:00:00 2001
> From: Anoop C S <anoopcs at redhat.com>
> Date: Tue, 18 Sep 2018 21:53:40 +0530
> Subject: [PATCH 1/2] s3/locking: Fix assertion check on lock reference count
> 
> lock_ref_count will always hold the old value prior to change. By the
> time current assertion check succeeds lock_ref_count would have already
> reached INT32_MAX. Therefore modifying this assertion to check for a
> value less than i.e, (INT32_MAX - 1).

Is INT32_MAX not supported as a value?
Is INT32_MAX - 1 the largest allowed value?
Note we are already asserting '<', not '<='.


> Signed-off-by: Anoop C S <anoopcs at redhat.com>
> ---
>  source3/locking/posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/source3/locking/posix.c b/source3/locking/posix.c
> index 79c33cfb0fa..1ee82b76f28 100644
> --- a/source3/locking/posix.c
> +++ b/source3/locking/posix.c
> @@ -429,7 +429,7 @@ static void increment_lock_ref_count(const files_struct *fsp)
>  		&lock_ref_count, 1);
>  
>  	SMB_ASSERT(NT_STATUS_IS_OK(status));
> -	SMB_ASSERT(lock_ref_count < INT32_MAX);
> +	SMB_ASSERT(lock_ref_count < (INT32_MAX - 1));
>  
>  	DEBUG(10,("lock_ref_count for file %s = %d\n",
>  		  fsp_str_dbg(fsp), (int)lock_ref_count));
> -- 
> 2.17.1
> 

> From 6113aeef666b10f0679d3bbb6a86e916bbdf5bc0 Mon Sep 17 00:00:00 2001
> From: Anoop C S <anoopcs at redhat.com>
> Date: Tue, 18 Sep 2018 21:53:54 +0530
> Subject: [PATCH 2/2] s3/locking: Fix logging of lock reference count
> 
> lock refernce count is always increased and reduced by a value of 1.
> But lock_ref_count variable holds the old value prior to change and
> was being logged wrongly under debug level 10. DEBUG statement must
> log lock_ref_count+1 and lock_ref_count-1 respectively when value
> gets increased and decreased.

This second patch LGTM.

Cheers - Michael

> Signed-off-by: Anoop C S <anoopcs at redhat.com>
> ---
>  source3/locking/posix.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/source3/locking/posix.c b/source3/locking/posix.c
> index 1ee82b76f28..787577408e5 100644
> --- a/source3/locking/posix.c
> +++ b/source3/locking/posix.c
> @@ -432,7 +432,7 @@ static void increment_lock_ref_count(const files_struct *fsp)
>  	SMB_ASSERT(lock_ref_count < (INT32_MAX - 1));
>  
>  	DEBUG(10,("lock_ref_count for file %s = %d\n",
> -		  fsp_str_dbg(fsp), (int)lock_ref_count));
> +		  fsp_str_dbg(fsp), (int)(lock_ref_count + 1)));
>  }
>  
>  /****************************************************************************
> @@ -453,7 +453,7 @@ static void decrement_lock_ref_count(const files_struct *fsp)
>  	SMB_ASSERT(lock_ref_count > 0);
>  
>  	DEBUG(10,("lock_ref_count for file %s = %d\n",
> -		  fsp_str_dbg(fsp), (int)lock_ref_count));
> +		  fsp_str_dbg(fsp), (int)(lock_ref_count - 1)));
>  }
>  
>  /****************************************************************************
> -- 
> 2.17.1
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180921/2105e587/signature.sig>