On Friday, 21 September 2018 13:23:29 CEST Michael Adam via samba-technical
wrote:
> 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));
> >
> > 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.
^^^^^^^^
typo :-)
> > 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)));
> >
> > }
> >
> > /************************************************************************
> > ****
--
Andreas Schneider asn at samba.org
Samba Team www.samba.org
GPG-ID: 8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D