Setting sin/sin6 port to 0 before ifa_ifwithaddr()

[Elad Efrat]

Hey

The netinet and netinet6 both have code like the following:

254 sin6->sin6_port = 0; /* yech... */
255 if ((in6p->in6p_flags & IN6P_FAITH) == 0 &&
256 (ia = ifa_ifwithaddr((struct sockaddr *)sin6)) == 0)
257 return (EADDRNOTAVAIL);

because at some point in the past ifa_ifwithaddr() used to do a
simple byte compare, including the port.

Perhaps we can get rid of it? Please see attached diff.

Thanks,

-e.

[Joerg Sonnenberger]

I don't think that that is necessary for your purposes. Also,
calling the comparison routine through a function pointer may have an
undesirable performance impact on some architectures (ARM?).

In fact, it should be a pessimation for almost all platforms compared
to a simple switch that covers the common cases and only falls back to a
function pointer list for non-AF_INET/non-AF_INET6.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[David Young]

Post by Elad Efrat
Hey
254 sin6->sin6_port = 0; /* yech... */
255 if ((in6p->in6p_flags & IN6P_FAITH) == 0 &&
256 (ia = ifa_ifwithaddr((struct sockaddr *)sin6)) == 0)
257 return (EADDRNOTAVAIL);
because at some point in the past ifa_ifwithaddr() used to do a
simple byte compare, including the port.
Perhaps we can get rid of it? Please see attached diff.
Thanks,
-e.
Index: netinet/in_pcb.c
===================================================================
RCS file: /usr/cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.130
diff -u -p -r1.130 in_pcb.c
--- netinet/in_pcb.c 18 Mar 2009 16:00:22 -0000 1.130
+++ netinet/in_pcb.c 13 Apr 2009 15:44:39 -0000
@@ -262,7 +262,6 @@ in_pcbbind(void *v, struct mbuf *nam, st
if (so->so_options & SO_REUSEADDR)
reuseport = SO_REUSEADDR|SO_REUSEPORT;
} else if (!in_nullhost(sin->sin_addr)) {
- sin->sin_port = 0; /* yech... */
INADDR_TO_IA(sin->sin_addr, ia);
/* check for broadcast addresses */
if (ia == NULL)
Index: netinet6/in6_pcb.c
===================================================================
RCS file: /usr/cvs/src/sys/netinet6/in6_pcb.c,v
retrieving revision 1.101
diff -u -p -r1.101 in6_pcb.c
--- netinet6/in6_pcb.c 18 Mar 2009 17:06:52 -0000 1.101
+++ netinet6/in6_pcb.c 13 Apr 2009 15:41:42 -0000
@@ -251,7 +251,6 @@ in6_pcbbind(void *v, struct mbuf *nam, s
} else if (!IN6_IS_ADDR_UNSPECIFIED(&sin6->sin6_addr)) {
struct ifaddr *ia = NULL;
- sin6->sin6_port = 0; /* yech... */
if ((in6p->in6p_flags & IN6P_FAITH) == 0 &&
(ia = ifa_ifwithaddr((struct sockaddr *)sin6)) == 0)
return (EADDRNOTAVAIL);

The part above looks ok.

Post by Elad Efrat
Index: netinet/in_proto.c
===================================================================
RCS file: /usr/cvs/src/sys/netinet/in_proto.c,v
retrieving revision 1.97
diff -u -p -r1.97 in_proto.c
--- netinet/in_proto.c 28 Feb 2009 18:31:12 -0000 1.97
+++ netinet/in_proto.c 13 Apr 2009 16:02:32 -0000
@@ -464,6 +464,7 @@ struct domain inetdomain = {
.dom_mowner = MOWNER_INIT("",""),
.dom_sa_cmpofs = offsetof(struct sockaddr_in, sin_addr),
.dom_sa_cmplen = sizeof(struct in_addr),
+ .dom_sockaddr_cmp = sockaddr_in_cmp,
.dom_sa_any = (const struct sockaddr *)&in_any,
.dom_sockaddr_const_addr = sockaddr_in_const_addr,
.dom_sockaddr_addr = sockaddr_in_addr,
Index: netinet6/in6_proto.c
===================================================================
RCS file: /usr/cvs/src/sys/netinet6/in6_proto.c,v
retrieving revision 1.84
diff -u -p -r1.84 in6_proto.c
--- netinet6/in6_proto.c 23 Mar 2009 18:43:20 -0000 1.84
+++ netinet6/in6_proto.c 13 Apr 2009 16:02:21 -0000
@@ -416,6 +416,7 @@ struct domain inet6domain = {
.dom_mowner = MOWNER_INIT("",""),
.dom_sa_cmpofs = offsetof(struct sockaddr_in6, sin6_addr),
.dom_sa_cmplen = sizeof(struct in6_addr),
+ .dom_sockaddr_cmp = sockaddr_in6_cmp,
.dom_sa_any = (const struct sockaddr *)&in6_any,
.dom_rtcache = LIST_HEAD_INITIALIZER(inet6domain.dom_rtcache)
};

I don't think that that is necessary for your purposes. Also,
calling the comparison routine through a function pointer may have an
undesirable performance impact on some architectures (ARM?).

Dave

[Elad Efrat]

Post by David Young

[changes to in{,6}_pcb.c]

The part above looks ok.

[changes to in{,6}_proto.c]

I don't think that that is necessary for your purposes. Also,
calling the comparison routine through a function pointer may have an
undesirable performance impact on some architectures (ARM?).

Thanks for reviewing, I'll go ahead and commit the first part -- you
are right that it's enough to compare the address part alone as the
proper offsets are used.

-e.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de