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

Jeremy Allison jra at samba.org
Tue Mar 6 23:22:03 UTC 2018


On Tue, Mar 06, 2018 at 01:15:57PM +0100, Swen Schillig via samba-technical wrote:
> Sorry, reply went to Andreas only.
> 
> -------- Forwarded Message --------
> From: Swen Schillig <swen at vnet.ibm.com>
> To: Andreas Schneider <asn at samba.org>
> Subject: Re: [PATCH] s3: Fix max indentation and max column
> Date: Tue, 06 Mar 2018 12:59:12 +0100
> 
> Hi Andreas
> 
> On Tue, 2018-03-06 at 11:56 +0100, Andreas Schneider wrote:
> > On Tuesday, 6 March 2018 11:43:13 CET Swen Schillig via samba-
> > technical wrote:
> > > Please review.... and push :-)
> > 
> > +		if (!forclose && !dptr->dir_hnd) {
> > +			if (dirhandles_open >= MAX_OPEN_DIRECTORIES)
> > +				dptr_idleoldest(sconn);
> > 
> > Please add brackets here! See CVE-2014-1266.
> 
> Done.
> 
> Thanks.
> 
> Cheers Swen

> From 3d418cd2213a3721bd4cc6185ad5d515fcbd724b 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 | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> index 6621b4ee387..4fad65c90d7 100644
> --- a/source3/smbd/dir.c
> +++ b/source3/smbd/dir.c
> @@ -159,29 +159,32 @@ 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) {
> +		if(dptr->dnum != key) {
> +			continue;
> +		}
> +
> +		if (!forclose && !dptr->dir_hnd) {
> +			if (dirhandles_open >= MAX_OPEN_DIRECTORIES) {
> +				dptr_idleoldest(sconn);
> +			}
> +			DEBUG(4,("dptr_get: Reopening dptr key %d\n",key));
> +
> +			dptr->dir_hnd = OpenDir(NULL, dptr->conn,
> +						dptr->smb_dname,
> +						dptr->wcard, dptr->attr);

As a formatting comment. Please don't change:

> -				if (!(dptr->dir_hnd = OpenDir(NULL,
> -							dptr->conn,
> -							dptr->smb_dname,
> -							dptr->wcard,
> -							dptr->attr))) {

to

> +			dptr->dir_hnd = OpenDir(NULL, dptr->conn,
> +						dptr->smb_dname,
> +						dptr->wcard, dptr->attr);

when reformatting. When calling a function one paramter per line is *much* easier to
read and modify than multiple paramerers per-line.

This is because if you need to change one parameter later, it makes it extremely
clear what it is you are changing in a future patchset.

I know it seems overly picky, but I'd like to NAK until I see a version
that looks like:

> +			dptr->dir_hnd = OpenDir(NULL,
> +						dptr->conn,
> +						dptr->smb_dname,
> +						dptr->wcard,
> +						dptr->attr);

instead. It's very close to being right, I want to make the
patch be beautiful as well :-) :-).

Thanks !

Jeremy.



More information about the samba-technical mailing list