[patch 10/13] net/socket.c::sys_bind() cleanup.

[patch 10/13] net/socket.c::sys_bind() cleanup.

[akpm]

From: "Luiz Fernando N. Capitulino" <lcapitulino@conectiva.com.br>

net/socket.c::sys_bind() is a bit complex function, the patch below makes
it more clear.

Signed-off-by: Luiz Capitulino <lcapitulino@conectiva.com.br>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/net/socket.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff -puN net/socket.c~net-socketcsys_bind-cleanup net/socket.c
--- 25/net/socket.c~net-socketcsys_bind-cleanup	2004-11-21 22:42:53.618530984 -0800
+++ 25-akpm/net/socket.c	2004-11-21 22:43:00.586471696 -0800
@@ -1286,18 +1286,23 @@ asmlinkage long sys_bind(int fd, struct 
 	char address[MAX_SOCK_ADDR];
 	int err;
 
-	if((sock = sockfd_lookup(fd,&err))!=NULL)
-	{
-		if((err=move_addr_to_kernel(umyaddr,addrlen,address))>=0) {
-			err = security_socket_bind(sock, (struct sockaddr *)address, addrlen);
-			if (err) {
-				sockfd_put(sock);
-				return err;
-			}
-			err = sock->ops->bind(sock, (struct sockaddr *)address, addrlen);
-		}
-		sockfd_put(sock);
-	}			
+	sock = sockfd_lookup(fd, &err);
+	if (!sock)
+		goto out;
+
+	err = move_addr_to_kernel(umyaddr, addrlen, address);
+	if (err)
+		goto out_put;
+
+	err = security_socket_bind(sock, (struct sockaddr *)address, addrlen);
+	if (err)
+		goto out_put;
+
+	err = sock->ops->bind(sock, (struct sockaddr *)address, addrlen);
+
+ out_put:
+	sockfd_put(sock);
+ out:
 	return err;
 }
 
_

Re: [patch 10/13] net/socket.c::sys_bind() cleanup.

[David S. Miller]

On Sun, 21 Nov 2004 22:43:56 -0800
akpm@osdl.org wrote:

> From: "Luiz Fernando N. Capitulino" <lcapitulino@conectiva.com.br>
> 
> net/socket.c::sys_bind() is a bit complex function, the patch below makes
> it more clear.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@conectiva.com.br>
> Signed-off-by: Andrew Morton <akpm@osdl.org>

This was commented on to be buggy, or at least change behavior.
The "if (err >= 0)" tests were changed to flat "if (err)" tests.

Re: [patch 10/13] net/socket.c::sys_bind() cleanup.

[Luiz Fernando Capitulino]

David S. Miller wrote:
> On Sun, 21 Nov 2004 22:43:56 -0800
> akpm@osdl.org wrote:
> 
> 
>>From: "Luiz Fernando N. Capitulino" <lcapitulino@conectiva.com.br>
>>
>>net/socket.c::sys_bind() is a bit complex function, the patch below makes
>>it more clear.
>>
>>Signed-off-by: Luiz Capitulino <lcapitulino@conectiva.com.br>
>>Signed-off-by: Andrew Morton <akpm@osdl.org>
> 
> 
> This was commented on to be buggy, or at least change behavior.
> The "if (err >= 0)" tests were changed to flat "if (err)" tests.

  It doesn't a buggy or change behaivor.

  What happens here is that move_addr_to_kernel() returns 0
success and -EINVAL or -EFAULT on error. Thus, change from
"if (err >= 0)" to "if (err)" is safe.

  Also, it was discussed and ACK'ed by James Morris:

http://lkml.org/lkml/2004/11/16/339

  The real problem here I've made a bad patch
description. I'll try to make it better next time.

  thanks,

-- 
Luiz Fernando N. Capitulino

Re: [patch 10/13] net/socket.c::sys_bind() cleanup.

[David Stevens]

It's just a style issue, so maybe you'll disagree, but I prefer:

        if (!sock)
                return err;

to your

        if (!sock)
                goto out;
...
out:
        return err;

        I think "return err" is more readable than "goto out/return err" 
for
that path and having another "return err" for the out-with-put case isn't 
a
terrible thing.

                                +-DLS

Re: [patch 10/13] net/socket.c::sys_bind() cleanup.

[Herbert Xu]

David Stevens <dlstevens@us.ibm.com> wrote:
> It's just a style issue, so maybe you'll disagree, but I prefer:
> 
>        if (!sock)
>                return err;
> 
> to your
> 
>        if (!sock)
>                goto out;
> ...
> out:
>        return err;
> 
>        I think "return err" is more readable than "goto out/return err" 
> for
> that path and having another "return err" for the out-with-put case isn't 
> a
> terrible thing.

When used in isolation you would be right.  However when there
are multiple exit points then the goto is better because it
means that we have only one exit path.  When you have multiple
exit paths it's very easy to get memory leaks and missing unlock's.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt