[Fwd: Re: [PATCH] s3: Fix max indentation and max column]

Christof Schmitt cs at samba.org
Mon Mar 12 18:42:17 UTC 2018


On Fri, Mar 09, 2018 at 04:31:29PM -0800, Jeremy Allison via samba-technical wrote:
> On Fri, Mar 09, 2018 at 04:00:27PM -0700, Christof Schmitt wrote:
> > On Fri, Mar 09, 2018 at 02:11:23PM -0800, Jeremy Allison via samba-technical wrote:
> > > On Wed, Mar 07, 2018 at 08:04:54PM +0100, Swen Schillig wrote:
> > > > Jeremy, you're a bad man :-)
> > > > 
> > > > On Wed, 2018-03-07 at 09:16 -0800, Jeremy Allison wrote:
> > > > > > On Tue, 2018-03-06 at 15:22 -0800, Jeremy Allison wrote:
> > > > > > > 
> > > > > Well what you're doing here is also reformatting the original
> > > > > code (which curently matches my preferred pattern). So IMHO when
> > > > > removing a layer of indentation you can keep the existing formatting
> > > > > of the parameter list without violating the Samba coding rules.
> > > > > 
> > > > > Plus right now you have a NAK with the patten change, but
> > > > > only a *potential* NAK without the pattern change.
> > > > > 
> > > > > So the way forward is to re-submit this without the pattern
> > > > > change and I'll +1 and ask for a second Team member +1. If
> > > > > we get another NAK due to the patten change then we'll cross
> > > > > that bridge when we come to it.
> > > > > 
> > > > > In the meantime I'll work on getting README.Coding changed
> > > > > so the "correct" way of parameter passing is preferred :-).
> > > > > 
> > > > ...here we go...complying to new coding rules :-)
> > > > 
> > > > Cheers Swen
> > > 
> > > LGTM. Reviewed-by: Jeremy Allison <jra at samba.org>
> > > 
> > > Can I get a second Team reviewer ?
> > > 
> > > Jeremy.
> > > 
> > > > From 4e884e5a19779aacf77d6ffd3c971474eb356472 Mon Sep 17 00:00:00 2001
> > > > From: Swen Schillig <swen at vnet.ibm.com>
> > > > Date: Mon, 5 Mar 2018 12:55:23 +0100
> > > > Subject: [PATCH] s3: Fix max indentation and max column
> > > > 
> > > > Minor cleanup reducing the max indentation level and max column length.
> > > > 
> > > > Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
> > > > ---
> > > >  source3/smbd/dir.c | 45 +++++++++++++++++++++++++--------------------
> > > >  1 file changed, 25 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> > > > index 6621b4ee387..a4045918cc3 100644
> > > > --- a/source3/smbd/dir.c
> > > > +++ b/source3/smbd/dir.c
> > > > @@ -159,29 +159,34 @@ static struct dptr_struct *dptr_get(struct smbd_server_connection *sconn,
> > > >  				    int key, bool forclose)
> > > >  {
> > > >  	struct dptr_struct *dptr;
> > > > +	const int dirhandles_open = sconn->searches.dirhandles_open;
> > > >  
> > > > -	for(dptr = sconn->searches.dirptrs; dptr; dptr = dptr->next) {
> > > > -		if(dptr->dnum == key) {
> > > > -			if (!forclose && !dptr->dir_hnd) {
> > > > -				if (sconn->searches.dirhandles_open >= MAX_OPEN_DIRECTORIES)
> > > > -					dptr_idleoldest(sconn);
> > > > -				DEBUG(4,("dptr_get: Reopening dptr key %d\n",key));
> > > > -
> > > > -				if (!(dptr->dir_hnd = OpenDir(NULL,
> > > > -							dptr->conn,
> > > > -							dptr->smb_dname,
> > > > -							dptr->wcard,
> > > > -							dptr->attr))) {
> > > > -					DEBUG(4,("dptr_get: Failed to "
> > > > -						"open %s (%s)\n",
> > > > -						dptr->smb_dname->base_name,
> > > > -						strerror(errno)));
> > > > -					return NULL;
> > > > -				}
> > > > +	for (dptr = sconn->searches.dirptrs; dptr; dptr = dptr->next) {

Should the condition compare explicitly against NULL?
dptr != NULL

> > > > +		if(dptr->dnum != key) {
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		if (!forclose && !dptr->dir_hnd) {

Add an explicit check for NULL:
if (!forclose && dptr->dir_hnd == NULL) {


> > > > +			if (dirhandles_open >= MAX_OPEN_DIRECTORIES) {
> > > > +				dptr_idleoldest(sconn);
> > > > +			}
> > > > +			DEBUG(4,("dptr_get: Reopening dptr key %d\n",key));
> > 
> > Should we update the debug statements while touching the code?
> > That could become:
> > DBG_INFO("Reopening dptr key %d\n", key);
> 
> Yeah, fair enough if we're updating the code
> we should probably do that.
> 
> > > > +
> > > > +			dptr->dir_hnd = OpenDir(NULL,
> > > > +						dptr->conn,
> > > > +						dptr->smb_dname,
> > > > +						dptr->wcard,
> > > > +						dptr->attr);
> > > > +
> > > > +			if (dptr->dir_hnd == NULL) {
> > > > +				DEBUG(4,("dptr_get: Failed to open %s (%s)\n",
> > > > +				      dptr->smb_dname->base_name,
> > > > +				      strerror(errno)));
> > 
> > DBG_NOTICE("Failed to open ...
> 
> If I do these changes will you +1 it ?

I took a closer look. If we touch the code anyway, the pointer checks
could also explicitly check for NULL.

Otherwise this looks good.

+1 from me with these changes included.

Christof



More information about the samba-technical mailing list