| Message ID | 1307740096-19933-4-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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 --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 */