[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