netdev - Re: [PATCH net-next 10/10] tipc: refactor accept() code for improved readability

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121207195629.GB27639@windriver.com>
Date:	Fri, 7 Dec 2012 14:56:29 -0500
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	Neil Horman <nhorman@...driver.com>
CC:	David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>,
	Jon Maloy <jon.maloy@...csson.com>
Subject: Re: [PATCH net-next 10/10] tipc: refactor accept() code for improved
 readability

[Re: [PATCH net-next 10/10] tipc: refactor accept() code for improved readability] On 07/12/2012 (Fri 14:42) Neil Horman wrote:

> On Fri, Dec 07, 2012 at 09:28:18AM -0500, Paul Gortmaker wrote:
> > In TIPC's accept() routine, there is a large block of code relating
> > to initialization of a new socket, all within an if condition checking
> > if the allocation succeeded.
> > 
> > Here, we simply factor out that init code within the accept() function
> > to its own separate function, which improves readability, and makes
> > it easier to track which lock_sock() calls are operating on existing
> > vs. new sockets.
> > 
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.com>
> > ---
> >  net/tipc/socket.c | 93 +++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 49 insertions(+), 44 deletions(-)
> > 
> > diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> > index 38613cf..56661c8 100644
> > --- a/net/tipc/socket.c
> > +++ b/net/tipc/socket.c
> > @@ -1507,6 +1507,53 @@ static int listen(struct socket *sock, int len)
> >  	return res;
> >  }
> >  
> > +static void tipc_init_socket(struct sock *sk, struct socket *new_sock,
> > +			     struct sk_buff *buf)
> > +{
> Can you rename this to something more specific to its purpose?  tipc_init_socket
> makes me wonder why you're not calling it internally from tipc_create.  This
> seems more like a tipc_init_accept_sock, or some such.  Alternatively, since
> you're ony using it from your accept call, you might consider not factoring it
> out at all, and just reversing the logic in your accept function so that you do:
> 
>  res = tipc_create(...)
>  if (res)
> 	goto exit;
>  <rest of tipc_init_socket goes here>
> 
> That gives you the same level of readability, without the additional potential
> call instruction, plus you put the unlikely case inside the if conditional.

I'm inclined to do the latter and just flip the sense of the "if" since
I already scratched my head trying to think of a good name (and
apparently failed in the end).

Thanks,
Paul.

> 
> Neil
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html