diff mbox

[v3,3/6] nfs-utils: Implement srcaddr binding in rpc_socket

Message ID 1307740096-19933-4-git-send-email-greearb@candelatech.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Greear June 10, 2011, 9:08 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

This implements the actual binding, if we are passed
a non-null local_ip structure.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 5652f6c... 4bcbdf0... M	support/nfs/rpc_socket.c
 support/nfs/rpc_socket.c |   59 +++++++++++++++++++++++++++++++++++----------
 1 files changed, 46 insertions(+), 13 deletions(-)

Comments

Chuck Lever June 10, 2011, 10:06 p.m. UTC | #1 
On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote:

> From: Ben Greear <greearb@candelatech.com>
> 
> This implements the actual binding, if we are passed
> a non-null local_ip structure.

Why not _always_ pass a valid local_ip structure, and simply set .addr to an appropriate ANYADDR by default?  Then .is_set wouldn't be necessary, would it?  It would also simplify the logic in nfs_validate_options().

> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> :100644 100644 5652f6c... 4bcbdf0... M	support/nfs/rpc_socket.c
> support/nfs/rpc_socket.c |   59 +++++++++++++++++++++++++++++++++++----------
> 1 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/support/nfs/rpc_socket.c b/support/nfs/rpc_socket.c
> index 5652f6c..4bcbdf0 100644
> --- a/support/nfs/rpc_socket.c
> +++ b/support/nfs/rpc_socket.c
> @@ -115,6 +115,8 @@ static CLIENT *nfs_get_localclient(const struct sockaddr *sap,
> static int nfs_bind(const int sock, const sa_family_t family,
> 		    struct local_bind_info *local_ip)
> {
> +	struct sockaddr *sa = NULL;
> +	socklen_t salen = 0;
> 	struct sockaddr_in sin = {
> 		.sin_family		= AF_INET,
> 		.sin_addr.s_addr	= htonl(INADDR_ANY),
> @@ -124,15 +126,26 @@ static int nfs_bind(const int sock, const sa_family_t family,
> 		.sin6_addr		= IN6ADDR_ANY_INIT,
> 	};
> 
> -	switch (family) {
> -	case AF_INET:
> -		return bind(sock, (struct sockaddr *)(char *)&sin,
> -					(socklen_t)sizeof(sin));
> -	case AF_INET6:
> -		return bind(sock, (struct sockaddr *)(char *)&sin6,
> -					(socklen_t)sizeof(sin6));
> +	if (local_ip && local_ip->is_set) {
> +		sa = &local_ip->addr.sa;
> +		salen = local_ip->addrlen;
> +	} else {
> +		switch (family) {
> +		case AF_INET:
> +			sa = (struct sockaddr *)&sin;
> +			salen = sizeof(sin);
> +			break;
> +		case AF_INET6:
> +			sa = (struct sockaddr *)&sin6;
> +			salen = sizeof(sin6);
> +		default:
> +			break;
> +		}
> 	}
> 
> +	if (sa)
> +		return bind(sock, sa, salen);
> +
> 	errno = EAFNOSUPPORT;
> 	return -1;
> }
> @@ -148,6 +161,7 @@ static int nfs_bind(const int sock, const sa_family_t family,
> static int nfs_bindresvport(const int sock, const sa_family_t family,
> 			    struct local_bind_info *local_ip)
> {
> +	struct sockaddr *sa = NULL;
> 	struct sockaddr_in sin = {
> 		.sin_family		= AF_INET,
> 		.sin_addr.s_addr	= htonl(INADDR_ANY),
> @@ -157,13 +171,23 @@ static int nfs_bindresvport(const int sock, const sa_family_t family,
> 		.sin6_addr		= IN6ADDR_ANY_INIT,
> 	};
> 
> -	switch (family) {
> -	case AF_INET:
> -		return bindresvport_sa(sock, (struct sockaddr *)(char *)&sin);
> -	case AF_INET6:
> -		return bindresvport_sa(sock, (struct sockaddr *)(char *)&sin6);
> +	if (local_ip && local_ip->is_set) {
> +		sa = &local_ip->addr.sa;
> +	} else {
> +		switch (family) {
> +		case AF_INET:
> +			sa = (struct sockaddr *)&sin;
> +			break;
> +		case AF_INET6:
> +			sa = (struct sockaddr *)&sin6;
> +		default:
> +			break;
> +		}
> 	}
> 
> +	if (sa)
> +		return bindresvport_sa(sock, sa);
> +
> 	errno = EAFNOSUPPORT;
> 	return -1;
> }
> @@ -179,12 +203,21 @@ static int nfs_bindresvport(const int sock, const sa_family_t family,
> static int nfs_bindresvport(const int sock, const sa_family_t family,
> 			    struct local_bind_info *local_ip)
> {
> +	struct sockaddr_in laddr;
> 	if (family != AF_INET) {
> 		errno = EAFNOSUPPORT;
> 		return -1;
> 	}
> 
> -	return bindresvport(sock, NULL);
> +	laddr.sin_family = family;
> +	laddr.sin_port = 0;
> +	if (local_ip && local_ip->is_set) {
> +		struct sockaddr_in *si = &local_ip->addr.s4;
> +		laddr.sin_addr.s_addr = si->sin_addr.s_addr;
> +	} else {
> +		laddr.sin_addr.s_addr = htonl(INADDR_ANY);
> +	}
> +	return bindresvport(sock, &laddr);
> }
> 
> #endif	/* !HAVE_LIBTIRPC */
> -- 
> 1.7.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear June 10, 2011, 10:19 p.m. UTC | #2 
On 06/10/2011 03:06 PM, Chuck Lever wrote:
>
> On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote:
>
>> From: Ben Greear<greearb@candelatech.com>
>>
>> This implements the actual binding, if we are passed
>> a non-null local_ip structure.
>
> Why not _always_ pass a valid local_ip structure, and simply set .addr to an appropriate ANYADDR by default?  Then .is_set wouldn't be necessary, would it?  It would also simplify the logic in nfs_validate_options().

I like it as is because almost none of the new code is actually
used unless users pass in the srcaddr= option.  So, if I *did*
introduce any bugs, hopefully they would be limited to users
of the new option, and not a real regression.

Maybe after the srcaddr= code is used a bit I could go back and
do that cleanup.

But, I don't feel strongly about it, so if you think it's
worth the bother, I'll try changing the code as you suggest.

Thanks,
Ben
Chuck Lever June 10, 2011, 10:37 p.m. UTC | #3 
On Jun 10, 2011, at 6:19 PM, Ben Greear wrote:

> On 06/10/2011 03:06 PM, Chuck Lever wrote:
>> 
>> On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote:
>> 
>>> From: Ben Greear<greearb@candelatech.com>
>>> 
>>> This implements the actual binding, if we are passed
>>> a non-null local_ip structure.
>> 
>> Why not _always_ pass a valid local_ip structure, and simply set .addr to an appropriate ANYADDR by default?  Then .is_set wouldn't be necessary, would it?  It would also simplify the logic in nfs_validate_options().
> 
> I like it as is because almost none of the new code is actually
> used unless users pass in the srcaddr= option.  So, if I *did*
> introduce any bugs, hopefully they would be limited to users
> of the new option, and not a real regression.

It should be pretty obvious if something here breaks.

> Maybe after the srcaddr= code is used a bit I could go back and
> do that cleanup.
> 
> But, I don't feel strongly about it, so if you think it's
> worth the bother, I'll try changing the code as you suggest.

In the long-term, if my suggestion works out, this code would be simpler, and to me that's better than the risk of a little short-term instability.
Ben Greear June 10, 2011, 10:50 p.m. UTC | #4 
On 06/10/2011 03:37 PM, Chuck Lever wrote:
>
> On Jun 10, 2011, at 6:19 PM, Ben Greear wrote:
>
>> On 06/10/2011 03:06 PM, Chuck Lever wrote:
>>>
>>> On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote:
>>>
>>>> From: Ben Greear<greearb@candelatech.com>
>>>>
>>>> This implements the actual binding, if we are passed
>>>> a non-null local_ip structure.
>>>
>>> Why not _always_ pass a valid local_ip structure, and simply set .addr to an appropriate ANYADDR by default?  Then .is_set wouldn't be necessary, would it?  It would also simplify the logic in nfs_validate_options().
>>
>> I like it as is because almost none of the new code is actually
>> used unless users pass in the srcaddr= option.  So, if I *did*
>> introduce any bugs, hopefully they would be limited to users
>> of the new option, and not a real regression.
>
> It should be pretty obvious if something here breaks.
>
>> Maybe after the srcaddr= code is used a bit I could go back and
>> do that cleanup.
>>
>> But, I don't feel strongly about it, so if you think it's
>> worth the bother, I'll try changing the code as you suggest.
>
> In the long-term, if my suggestion works out, this code would be simpler, and to me that's better than the risk of a little short-term instability.

Do you mean always make sure it is not NULL as well?

That would complicate code everywhere I currently pass NULL in for local_ip
(like in methods that don't care about binding, non stropts logic, etc).
I think that would cause more harm than good.

I could add return value to the parse method instead of relying on
is_set (and just pass in NULL instead of &local_ip if we didn't have the srcaddr=
option) if you think that is cleaner, but I don't think it will simplify things very
much...

Thanks,
Ben
diff mbox

Patch

diff --git a/support/nfs/rpc_socket.c b/support/nfs/rpc_socket.c
index 5652f6c..4bcbdf0 100644
--- a/support/nfs/rpc_socket.c
+++ b/support/nfs/rpc_socket.c
@@ -115,6 +115,8 @@  static CLIENT *nfs_get_localclient(const struct sockaddr *sap,
 static int nfs_bind(const int sock, const sa_family_t family,
 		    struct local_bind_info *local_ip)
 {
+	struct sockaddr *sa = NULL;
+	socklen_t salen = 0;
 	struct sockaddr_in sin = {
 		.sin_family		= AF_INET,
 		.sin_addr.s_addr	= htonl(INADDR_ANY),
@@ -124,15 +126,26 @@  static int nfs_bind(const int sock, const sa_family_t family,
 		.sin6_addr		= IN6ADDR_ANY_INIT,
 	};
 
-	switch (family) {
-	case AF_INET:
-		return bind(sock, (struct sockaddr *)(char *)&sin,
-					(socklen_t)sizeof(sin));
-	case AF_INET6:
-		return bind(sock, (struct sockaddr *)(char *)&sin6,
-					(socklen_t)sizeof(sin6));
+	if (local_ip && local_ip->is_set) {
+		sa = &local_ip->addr.sa;
+		salen = local_ip->addrlen;
+	} else {
+		switch (family) {
+		case AF_INET:
+			sa = (struct sockaddr *)&sin;
+			salen = sizeof(sin);
+			break;
+		case AF_INET6:
+			sa = (struct sockaddr *)&sin6;
+			salen = sizeof(sin6);
+		default:
+			break;
+		}
 	}
 
+	if (sa)
+		return bind(sock, sa, salen);
+
 	errno = EAFNOSUPPORT;
 	return -1;
 }
@@ -148,6 +161,7 @@  static int nfs_bind(const int sock, const sa_family_t family,
 static int nfs_bindresvport(const int sock, const sa_family_t family,
 			    struct local_bind_info *local_ip)
 {
+	struct sockaddr *sa = NULL;
 	struct sockaddr_in sin = {
 		.sin_family		= AF_INET,
 		.sin_addr.s_addr	= htonl(INADDR_ANY),
@@ -157,13 +171,23 @@  static int nfs_bindresvport(const int sock, const sa_family_t family,
 		.sin6_addr		= IN6ADDR_ANY_INIT,
 	};
 
-	switch (family) {
-	case AF_INET:
-		return bindresvport_sa(sock, (struct sockaddr *)(char *)&sin);
-	case AF_INET6:
-		return bindresvport_sa(sock, (struct sockaddr *)(char *)&sin6);
+	if (local_ip && local_ip->is_set) {
+		sa = &local_ip->addr.sa;
+	} else {
+		switch (family) {
+		case AF_INET:
+			sa = (struct sockaddr *)&sin;
+			break;
+		case AF_INET6:
+			sa = (struct sockaddr *)&sin6;
+		default:
+			break;
+		}
 	}
 
+	if (sa)
+		return bindresvport_sa(sock, sa);
+
 	errno = EAFNOSUPPORT;
 	return -1;
 }
@@ -179,12 +203,21 @@  static int nfs_bindresvport(const int sock, const sa_family_t family,
 static int nfs_bindresvport(const int sock, const sa_family_t family,
 			    struct local_bind_info *local_ip)
 {
+	struct sockaddr_in laddr;
 	if (family != AF_INET) {
 		errno = EAFNOSUPPORT;
 		return -1;
 	}
 
-	return bindresvport(sock, NULL);
+	laddr.sin_family = family;
+	laddr.sin_port = 0;
+	if (local_ip && local_ip->is_set) {
+		struct sockaddr_in *si = &local_ip->addr.s4;
+		laddr.sin_addr.s_addr = si->sin_addr.s_addr;
+	} else {
+		laddr.sin_addr.s_addr = htonl(INADDR_ANY);
+	}
+	return bindresvport(sock, &laddr);
 }
 
 #endif	/* !HAVE_LIBTIRPC */