Re: route: improve inet6_makenetandmask
On Sun, Jun 24, 2018 at 10:29:55PM +0200, Klemens Nanni wrote: > On Sun, Jun 24, 2018 at 10:09:26PM +0200, Florian Obser wrote: > > I don't understand why it's equivalent. > > > > prefixlen() seems to operate on so_mask while the only caller of > > inet6_makenetandmask() passes in so_dst. > > > > Assuming it is equivalent, inet6_makenetandmask() is now missnamed and > > I think we should just get rid of it. We can just call prefixlen() > > directly, the diff turns inet6_makenetandmask() into a weird wrapper > > around prefixlen(). Note that sep can't be NULL, so plen can't be > > NULL. > `sep' is NULL everytime the address string passed on the command line > does not contain "/". so `plen' is NULL and we set the prefixlen as it > wasn't specified in the first place. > oops, yes, I missread the calling side, sorry. -- I'm not entirely sure you are real.
Re: route: improve inet6_makenetandmask
On Sun, Jun 24, 2018 at 10:09:26PM +0200, Florian Obser wrote: > I don't understand why it's equivalent. > > prefixlen() seems to operate on so_mask while the only caller of > inet6_makenetandmask() passes in so_dst. > > Assuming it is equivalent, inet6_makenetandmask() is now missnamed and > I think we should just get rid of it. We can just call prefixlen() > directly, the diff turns inet6_makenetandmask() into a weird wrapper > around prefixlen(). Note that sep can't be NULL, so plen can't be > NULL. `sep' is NULL everytime the address string passed on the command line does not contain "/". so `plen' is NULL and we set the prefixlen as it wasn't specified in the first place.
Re: route: improve inet6_makenetandmask
On Sun, Jun 24, 2018 at 07:54:48PM +0200, Jeremie Courreges-Anglas wrote: > On Sun, Jun 24 2018, Klemens Nanni wrote: > > On Sun, Jun 24, 2018 at 04:34:01PM +0200, Jeremie Courreges-Anglas wrote: > >> So if I understand correctly, this diff does three things: > >> 1. shorten the code (remove the explicit mask handling from this > >>function) > >> 2. stop considering 2000::/3 as special per RFC3587 > >> 3. stop considering 2001:db8:: as a /64 prefix by default, but consider > >>it as a host (/128) > >> > >> 1. might be desirable, even though I find it hard to follow the code > >>flow. Perhaps the code should be more explicit and stop abusing > >>globals... > >> > >> 2. fine, but... > >> > >> 3. ... why should we stop assuming that the user really means to > >>configure a route for a /64 if the host id part is all-zeroes? Is > >>this really part of what has been deprecated by RFC3587? > > Without context there's no point in guessing, what makes you think /64 > > is the appropiate choice? There should be no assumptions being made at > > all since there's nothing backing up /64 any more than whatever bigger > > prefix you can think of. > > I would also prefer if this code didn't exist in the first place, but it > does. > > So, playing the devil's advocate: what would prevent people from making > such assumptions? /64 is the most common prefix length configured on > interfaces (at least in my experience). IPv6 is heavily geared towards > this assumption, there are even studies about it[0]. I would argue that > the current behavior tries to be helpful. On the other hand, how often > do you configure routes to a Subnet-Router anycast address[1]? > > I really think your diff goes in the right direction, so if no one shows > up with strong objections I'd suggest that you commit your diff soon > with my ok. But please do not justify item 3 with RFC3587. > > So... anyone else with strong feelings regarding this change? I don't understand why it's equivalent. prefixlen() seems to operate on so_mask while the only caller of inet6_makenetandmask() passes in so_dst. Assuming it is equivalent, inet6_makenetandmask() is now missnamed and I think we should just get rid of it. We can just call prefixlen() directly, the diff turns inet6_makenetandmask() into a weird wrapper around prefixlen(). Note that sep can't be NULL, so plen can't be NULL. > > >> I can understand that having the same behavior (host address is no > >> prefix length is specified) with v4 and v6 is desirable, but as benno > >> pointed out, some people might be relying on the current default > >> behavior. That's not a strong objection, but I'd like to know what's > >> the exact rationale behind this change. > > It removes the guess-work. Installing a route for 2001:db8:: should > > use /128 simply because that's what it is. Benno's -blackhole example > > demonstrates how dangerous implicit meanings and/or assumptions can be. > > Assumptions may not be good, but unless I missed something benno's > example[2] is a different issue and this diff doesn't address it, the > resulting route is still not what the user expects. > > [0] https://tools.ietf.org/html/rfc7421 "Analysis of the 64-bit Boundary > in IPv6 Addressing" > [1] https://tools.ietf.org/html/rfc4291#section-2.6.1 "Required Anycast > Address" > [2] https://marc.info/?l=openbsd-misc=152719815704699=2 > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE > -- I'm not entirely sure you are real.
Re: route: improve inet6_makenetandmask
On Sun, Jun 24, 2018 at 08:06:48PM +0200, Sebastian Benoit wrote: > And i think that > > route add -inet6 -prefixlen 56 2001:db10:: 2001:db9::5 As stated in the manual The implicit network mask generated in the AF_INET case can be overridden by making sure this option follows the destination parameter. -prefixlen is also available for a similar purpose, for IPv6/v4. > should be an error So behaviour is as documented. But I agree that `-prefixlen' should alwayas overwrite take precendence.
Re: route: improve inet6_makenetandmask
Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2018.06.24 19:54:48 +0200: > On Sun, Jun 24 2018, Klemens Nanni wrote: > > On Sun, Jun 24, 2018 at 04:34:01PM +0200, Jeremie Courreges-Anglas wrote: > >> So if I understand correctly, this diff does three things: > >> 1. shorten the code (remove the explicit mask handling from this > >>function) > >> 2. stop considering 2000::/3 as special per RFC3587 > >> 3. stop considering 2001:db8:: as a /64 prefix by default, but consider > >>it as a host (/128) > >> > >> 1. might be desirable, even though I find it hard to follow the code > >>flow. Perhaps the code should be more explicit and stop abusing > >>globals... > >> > >> 2. fine, but... > >> > >> 3. ... why should we stop assuming that the user really means to > >>configure a route for a /64 if the host id part is all-zeroes? Is > >>this really part of what has been deprecated by RFC3587? > > Without context there's no point in guessing, what makes you think /64 > > is the appropiate choice? There should be no assumptions being made at > > all since there's nothing backing up /64 any more than whatever bigger > > prefix you can think of. > > I would also prefer if this code didn't exist in the first place, but it > does. me too! > So, playing the devil's advocate: what would prevent people from making > such assumptions? /64 is the most common prefix length configured on > interfaces (at least in my experience). IPv6 is heavily geared towards > this assumption, there are even studies about it[0]. I would argue that > the current behavior tries to be helpful. On the other hand, how often > do you configure routes to a Subnet-Router anycast address[1]? > > I really think your diff goes in the right direction, so if no one shows > up with strong objections I'd suggest that you commit your diff soon > with my ok. But please do not justify item 3 with RFC3587. > > So... anyone else with strong feelings regarding this change? > > >> I can understand that having the same behavior (host address is no > >> prefix length is specified) with v4 and v6 is desirable, but as benno > >> pointed out, some people might be relying on the current default > >> behavior. That's not a strong objection, but I'd like to know what's > >> the exact rationale behind this change. > > It removes the guess-work. Installing a route for 2001:db8:: should > > use /128 simply because that's what it is. Benno's -blackhole example > > demonstrates how dangerous implicit meanings and/or assumptions can be. > > Assumptions may not be good, but unless I missed something benno's > example[2] is a different issue and this diff doesn't address it, the > resulting route is still not what the user expects. it is this issue. i think it can be commited, but please add something to current.html. And i think that route add -inet6 -prefixlen 56 2001:db10:: 2001:db9::5 should be an error (because right now it configures a 2001:db10::/64 route, and with your diff a /128). > [0] https://tools.ietf.org/html/rfc7421 "Analysis of the 64-bit Boundary > in IPv6 Addressing" > [1] https://tools.ietf.org/html/rfc4291#section-2.6.1 "Required Anycast > Address" > [2] https://marc.info/?l=openbsd-misc=152719815704699=2 > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >
Re: route: improve inet6_makenetandmask
On Sun, Jun 24 2018, Klemens Nanni wrote: > On Sun, Jun 24, 2018 at 04:34:01PM +0200, Jeremie Courreges-Anglas wrote: >> So if I understand correctly, this diff does three things: >> 1. shorten the code (remove the explicit mask handling from this >>function) >> 2. stop considering 2000::/3 as special per RFC3587 >> 3. stop considering 2001:db8:: as a /64 prefix by default, but consider >>it as a host (/128) >> >> 1. might be desirable, even though I find it hard to follow the code >>flow. Perhaps the code should be more explicit and stop abusing >>globals... >> >> 2. fine, but... >> >> 3. ... why should we stop assuming that the user really means to >>configure a route for a /64 if the host id part is all-zeroes? Is >>this really part of what has been deprecated by RFC3587? > Without context there's no point in guessing, what makes you think /64 > is the appropiate choice? There should be no assumptions being made at > all since there's nothing backing up /64 any more than whatever bigger > prefix you can think of. I would also prefer if this code didn't exist in the first place, but it does. So, playing the devil's advocate: what would prevent people from making such assumptions? /64 is the most common prefix length configured on interfaces (at least in my experience). IPv6 is heavily geared towards this assumption, there are even studies about it[0]. I would argue that the current behavior tries to be helpful. On the other hand, how often do you configure routes to a Subnet-Router anycast address[1]? I really think your diff goes in the right direction, so if no one shows up with strong objections I'd suggest that you commit your diff soon with my ok. But please do not justify item 3 with RFC3587. So... anyone else with strong feelings regarding this change? >> I can understand that having the same behavior (host address is no >> prefix length is specified) with v4 and v6 is desirable, but as benno >> pointed out, some people might be relying on the current default >> behavior. That's not a strong objection, but I'd like to know what's >> the exact rationale behind this change. > It removes the guess-work. Installing a route for 2001:db8:: should > use /128 simply because that's what it is. Benno's -blackhole example > demonstrates how dangerous implicit meanings and/or assumptions can be. Assumptions may not be good, but unless I missed something benno's example[2] is a different issue and this diff doesn't address it, the resulting route is still not what the user expects. [0] https://tools.ietf.org/html/rfc7421 "Analysis of the 64-bit Boundary in IPv6 Addressing" [1] https://tools.ietf.org/html/rfc4291#section-2.6.1 "Required Anycast Address" [2] https://marc.info/?l=openbsd-misc=152719815704699=2 -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: route: improve inet6_makenetandmask
On Sun, Jun 24, 2018 at 04:34:01PM +0200, Jeremie Courreges-Anglas wrote: > So if I understand correctly, this diff does three things: > 1. shorten the code (remove the explicit mask handling from this >function) > 2. stop considering 2000::/3 as special per RFC3587 > 3. stop considering 2001:db8:: as a /64 prefix by default, but consider >it as a host (/128) > > 1. might be desirable, even though I find it hard to follow the code >flow. Perhaps the code should be more explicit and stop abusing >globals... > > 2. fine, but... > > 3. ... why should we stop assuming that the user really means to >configure a route for a /64 if the host id part is all-zeroes? Is >this really part of what has been deprecated by RFC3587? Without context there's no point in guessing, what makes you think /64 is the appropiate choice? There should be no assumptions being made at all since there's nothing backing up /64 any more than whatever bigger prefix you can think of. > I can understand that having the same behavior (host address is no > prefix length is specified) with v4 and v6 is desirable, but as benno > pointed out, some people might be relying on the current default > behavior. That's not a strong objection, but I'd like to know what's > the exact rationale behind this change. It removes the guess-work. Installing a route for 2001:db8:: should use /128 simply because that's what it is. Benno's -blackhole example demonstrates how dangerous implicit meanings and/or assumptions can be.
Re: route: improve inet6_makenetandmask
On Sun, Jun 24, 2018 at 04:34:01PM +0200, Jeremie Courreges-Anglas wrote:
> 3. ... why should we stop assuming that the user really means to
>configure a route for a /64 if the host id part is all-zeroes? Is
>this really part of what has been deprecated by RFC3587?
>
> I can understand that having the same behavior (host address is no
> prefix length is specified) with v4 and v6 is desirable, but as benno
> pointed out, some people might be relying on the current default
> behavior. That's not a strong objection, but I'd like to know what's
> the exact rationale behind this change.
>
> An alternate way of fixing item 2 would be to keep the current behavior
> but extend it to any foo:bar:: address, not just to addresses within
> 2000::/3.
>
With your diff you break the ULA address space where
route add fc01:db8:: ::1
results in fc01:db8::/64 being inserted instead of the /128 :)
>
> Index: route.c
> ===
> RCS file: /d/cvs/src/sbin/route/route.c,v
> retrieving revision 1.215
> diff -u -p -p -u -r1.215 route.c
> --- route.c 18 Jun 2018 09:17:06 - 1.215
> +++ route.c 24 Jun 2018 14:31:46 -
> @@ -792,7 +792,7 @@ inet_makenetandmask(u_int32_t net, struc
> int
> inet6_makenetandmask(struct sockaddr_in6 *sin6, char *plen)
> {
> - struct in6_addr in6;
> + static const struct in6_addr zero_in6;
> const char *errstr;
> int i, len, q, r;
>
> @@ -800,12 +800,9 @@ inet6_makenetandmask(struct sockaddr_in6
> if (IN6_IS_ADDR_UNSPECIFIED(>sin6_addr) &&
> sin6->sin6_scope_id == 0) {
> plen = "0";
> - } else if ((sin6->sin6_addr.s6_addr[0] & 0xe0) == 0x20) {
> - /* aggregatable global unicast - RFC2374 */
> - memset(, 0, sizeof(in6));
> - if (!memcmp(>sin6_addr.s6_addr[8],
> - _addr[8], 8))
> - plen = "64";
> + } else if (!memcmp(>sin6_addr.s6_addr[8],
> + _in6.s6_addr[8], 8)) {
> + plen = "64";
> }
> }
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
Re: route: improve inet6_makenetandmask
On Mon, Jun 11 2018, Klemens Nanni wrote:
> Here's a new diff that removes the duplicate parsing bits as mentioned
> before but leaves masking the address to mask_addr() instead of doing it
> manually.
>
> Furthermore, it also stops route(8) from assuming address strings
> without explicit prefix length to be /64.
>
> The old behaviour described in RFC 2374 from 1998 is obsolete as per
> RFC 3587 which states
>
> RFC 2374 was the definition of addresses for Format Prefix 001
> (2000::/3) which is formally made historic by this document. Even
> though currently only 2000::/3 is being delegated by the IANA,
> implementations should not make any assumptions about 2000::/3 being
> special. In the future, the IANA might be directed to delegate
> currently unassigned portions of the IPv6 address space for the
> purpose of Global Unicast as well.
>
> This was brought to my attention by the (recent) misc@ thread
> "Confusing IPv6 route(8) results"[0] in which benno@ mentioned an
> important side effect when it comes to using `-prefixlen' before the
> address string since order is relavant here and the following would
> therefore blackhole a single /128 with after my patch applied:
>
> # route add -inet6 -prefixlen 56 2001:db8:: ::1 -blackhole
>
> Thus, I'll put a note into current.html in case this change should go in.
>
> FWIW, both FreeBSD and NetBSD already use `prefixlen()' and do not mask
> the address in `inet6_makenetandmask()'. NetBSD still assumes /64 as per
> RFC 2374 while FreeBSD does the same already I'm aiming for.
>
> Regress passes, no regressions found when manually adding/removing
> routes.
>
> Feedback? Objections? OK?
Sorry for not caring about this sooner. Probably because it looked like
a can of worms...
So if I understand correctly, this diff does three things:
1. shorten the code (remove the explicit mask handling from this
function)
2. stop considering 2000::/3 as special per RFC3587
3. stop considering 2001:db8:: as a /64 prefix by default, but consider
it as a host (/128)
1. might be desirable, even though I find it hard to follow the code
flow. Perhaps the code should be more explicit and stop abusing
globals...
2. fine, but...
3. ... why should we stop assuming that the user really means to
configure a route for a /64 if the host id part is all-zeroes? Is
this really part of what has been deprecated by RFC3587?
I can understand that having the same behavior (host address is no
prefix length is specified) with v4 and v6 is desirable, but as benno
pointed out, some people might be relying on the current default
behavior. That's not a strong objection, but I'd like to know what's
the exact rationale behind this change.
An alternate way of fixing item 2 would be to keep the current behavior
but extend it to any foo:bar:: address, not just to addresses within
2000::/3.
Index: route.c
===
RCS file: /d/cvs/src/sbin/route/route.c,v
retrieving revision 1.215
diff -u -p -p -u -r1.215 route.c
--- route.c 18 Jun 2018 09:17:06 - 1.215
+++ route.c 24 Jun 2018 14:31:46 -
@@ -792,7 +792,7 @@ inet_makenetandmask(u_int32_t net, struc
int
inet6_makenetandmask(struct sockaddr_in6 *sin6, char *plen)
{
- struct in6_addr in6;
+ static const struct in6_addr zero_in6;
const char *errstr;
int i, len, q, r;
@@ -800,12 +800,9 @@ inet6_makenetandmask(struct sockaddr_in6
if (IN6_IS_ADDR_UNSPECIFIED(>sin6_addr) &&
sin6->sin6_scope_id == 0) {
plen = "0";
- } else if ((sin6->sin6_addr.s6_addr[0] & 0xe0) == 0x20) {
- /* aggregatable global unicast - RFC2374 */
- memset(, 0, sizeof(in6));
- if (!memcmp(>sin6_addr.s6_addr[8],
- _addr[8], 8))
- plen = "64";
+ } else if (!memcmp(>sin6_addr.s6_addr[8],
+ _in6.s6_addr[8], 8)) {
+ plen = "64";
}
}
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: route: improve inet6_makenetandmask
On Mon, Jun 11, 2018 at 01:09:17AM +0200, Klemens Nanni wrote: > Feedback? Objections? OK? OK denis so far, anyone else?
Re: route: improve inet6_makenetandmask
Here's a new diff that removes the duplicate parsing bits as mentioned
before but leaves masking the address to mask_addr() instead of doing it
manually.
Furthermore, it also stops route(8) from assuming address strings
without explicit prefix length to be /64.
The old behaviour described in RFC 2374 from 1998 is obsolete as per
RFC 3587 which states
RFC 2374 was the definition of addresses for Format Prefix 001
(2000::/3) which is formally made historic by this document. Even
though currently only 2000::/3 is being delegated by the IANA,
implementations should not make any assumptions about 2000::/3 being
special. In the future, the IANA might be directed to delegate
currently unassigned portions of the IPv6 address space for the
purpose of Global Unicast as well.
This was brought to my attention by the (recent) misc@ thread
"Confusing IPv6 route(8) results"[0] in which benno@ mentioned an
important side effect when it comes to using `-prefixlen' before the
address string since order is relavant here and the following would
therefore blackhole a single /128 with after my patch applied:
# route add -inet6 -prefixlen 56 2001:db8:: ::1 -blackhole
Thus, I'll put a note into current.html in case this change should go in.
FWIW, both FreeBSD and NetBSD already use `prefixlen()' and do not mask
the address in `inet6_makenetandmask()'. NetBSD still assumes /64 as per
RFC 2374 while FreeBSD does the same already I'm aiming for.
Regress passes, no regressions found when manually adding/removing
routes.
Feedback? Objections? OK?
0: https://marc.info/?l=openbsd-misc=152712936731762
Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.214
diff -u -p -r1.214 route.c
--- route.c 1 May 2018 18:14:10 - 1.214
+++ route.c 10 Jun 2018 22:35:43 -
@@ -786,50 +786,19 @@ inet_makenetandmask(u_int32_t net, struc
sin->sin_len = 1 + cp - (char *)sin;
}
-/*
- * XXX the function may need more improvement...
- */
int
inet6_makenetandmask(struct sockaddr_in6 *sin6, char *plen)
{
- struct in6_addr in6;
- const char *errstr;
- int i, len, q, r;
-
- if (NULL==plen) {
+ if (!plen) {
if (IN6_IS_ADDR_UNSPECIFIED(>sin6_addr) &&
- sin6->sin6_scope_id == 0) {
+ sin6->sin6_scope_id == 0)
plen = "0";
- } else if ((sin6->sin6_addr.s6_addr[0] & 0xe0) == 0x20) {
- /* aggregatable global unicast - RFC2374 */
- memset(, 0, sizeof(in6));
- if (!memcmp(>sin6_addr.s6_addr[8],
- _addr[8], 8))
- plen = "64";
- }
}
- if (!plen || strcmp(plen, "128") == 0)
+ if (!plen || prefixlen(AF_INET6, plen))
return (1);
- else {
- rtm_addrs |= RTA_NETMASK;
- prefixlen(AF_INET6, plen);
-
- len = strtonum(plen, 0, 128, );
- if (errstr)
- errx(1, "prefixlen %s is %s", plen, errstr);
-
- q = (128-len) >> 3;
- r = (128-len) & 7;
- i = 15;
- while (q-- > 0)
- sin6->sin6_addr.s6_addr[i--] = 0;
- if (r > 0)
- sin6->sin6_addr.s6_addr[i] &= 0xff << r;
-
- return (0);
- }
+ return (0);
}
/*
===
Stats: --- 35 lines 798 chars
Stats: +++ 4 lines 100 chars
Stats: -31 lines
Stats: -698 chars
Re: route: improve inet6_makenetandmask
On Sun, May 27, 2018 at 03:13:04AM +0200, Klemens Nanni wrote: > On Sat, May 26, 2018 at 11:49:29PM +0200, Denis Fondras wrote: > > Not related to this diff but RFC2374 has been made obsolete by RFC3587 for > > some > > years : "implementations should not make any assumptions about 2000::/3 > > being > > special". I think we can simplify this "else if" :) > Agreed, although this should probably go into a separate diff; Your post > on misc@ made me look at this in the first place, so I refrained from > taking over your work. > > > > + rtm_addrs |= RTA_NETMASK; > > > > Can't we consider this done in prefixlen() ? > Yes. Missed to remove it from here, thanks. > > Updated diff below without the null and curly bracket cosmetics to avoid > churn (and leave it for the RFC 3587 update around there). Bump. Any thoughts on this?
Re: route: improve inet6_makenetandmask
On Sat, May 26, 2018 at 11:49:29PM +0200, Denis Fondras wrote:
> Not related to this diff but RFC2374 has been made obsolete by RFC3587 for
> some
> years : "implementations should not make any assumptions about 2000::/3 being
> special". I think we can simplify this "else if" :)
Agreed, although this should probably go into a separate diff; Your post
on misc@ made me look at this in the first place, so I refrained from
taking over your work.
> > + rtm_addrs |= RTA_NETMASK;
>
> Can't we consider this done in prefixlen() ?
Yes. Missed to remove it from here, thanks.
Updated diff below without the null and curly bracket cosmetics to avoid
churn (and leave it for the RFC 3587 update around there).
Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.214
diff -u -p -r1.214 route.c
--- route.c 1 May 2018 18:14:10 - 1.214
+++ route.c 27 May 2018 01:04:36 -
@@ -786,15 +786,11 @@ inet_makenetandmask(u_int32_t net, struc
sin->sin_len = 1 + cp - (char *)sin;
}
-/*
- * XXX the function may need more improvement...
- */
int
inet6_makenetandmask(struct sockaddr_in6 *sin6, char *plen)
{
struct in6_addr in6;
- const char *errstr;
- int i, len, q, r;
+ int i;
if (NULL==plen) {
if (IN6_IS_ADDR_UNSPECIFIED(>sin6_addr) &&
@@ -809,27 +805,13 @@ inet6_makenetandmask(struct sockaddr_in6
}
}
- if (!plen || strcmp(plen, "128") == 0)
+ if (!plen || prefixlen(AF_INET6, plen))
return (1);
- else {
- rtm_addrs |= RTA_NETMASK;
- prefixlen(AF_INET6, plen);
-
- len = strtonum(plen, 0, 128, );
- if (errstr)
- errx(1, "prefixlen %s is %s", plen, errstr);
-
- q = (128-len) >> 3;
- r = (128-len) & 7;
- i = 15;
-
- while (q-- > 0)
- sin6->sin6_addr.s6_addr[i--] = 0;
- if (r > 0)
- sin6->sin6_addr.s6_addr[i] &= 0xff << r;
- return (0);
- }
+ for (i = 0; i < 16; ++i)
+ sin6->sin6_addr.s6_addr[i] &= so_mask.sin6.sin6_addr.s6_addr[i];
+
+ return (0);
}
/*
Re: route: improve inet6_makenetandmask
On Sat, May 26, 2018 at 12:54:03PM +0200, Klemens Nanni wrote:
> inet6_makenetandmask() parses the provided prefix length `plen' twice:
> first from inside prefixlen() which sets the mask, then right again
> doing almost the same dance to find out which bits to zero in the
> provided address.
>
> But once prefixlen() set so_mask, we can just use it to actually mask
> the address.
>
> prefixlen() returns `(len == max)' where `max = 128' for `AF_INET6', so
> it's equivalent to the strcmp() call.
>
> Regress tests pass on amd64, also no issue after manually adding,
> testing and removing routes on my machine.
>
> Feedback? OK?
>
> Index: route.c
> ===
> RCS file: /cvs/src/sbin/route/route.c,v
> retrieving revision 1.214
> diff -u -p -r1.214 route.c
> --- route.c 1 May 2018 18:14:10 - 1.214
> +++ route.c 26 May 2018 10:49:50 -
> @@ -786,21 +786,17 @@ inet_makenetandmask(u_int32_t net, struc
> sin->sin_len = 1 + cp - (char *)sin;
> }
>
> -/*
> - * XXX the function may need more improvement...
> - */
> int
> inet6_makenetandmask(struct sockaddr_in6 *sin6, char *plen)
> {
> struct in6_addr in6;
> - const char *errstr;
> - int i, len, q, r;
> + int i;
>
> - if (NULL==plen) {
> + if (!plen) {
> if (IN6_IS_ADDR_UNSPECIFIED(>sin6_addr) &&
> - sin6->sin6_scope_id == 0) {
> + sin6->sin6_scope_id == 0)
> plen = "0";
> - } else if ((sin6->sin6_addr.s6_addr[0] & 0xe0) == 0x20) {
> + else if ((sin6->sin6_addr.s6_addr[0] & 0xe0) == 0x20) {
> /* aggregatable global unicast - RFC2374 */
> memset(, 0, sizeof(in6));
> if (!memcmp(>sin6_addr.s6_addr[8],
Not related to this diff but RFC2374 has been made obsolete by RFC3587 for some
years : "implementations should not make any assumptions about 2000::/3 being
special". I think we can simplify this "else if" :)
> @@ -809,27 +805,14 @@ inet6_makenetandmask(struct sockaddr_in6
> }
> }
>
> - if (!plen || strcmp(plen, "128") == 0)
> + if (!plen || prefixlen(AF_INET6, plen))
> return (1);
> - else {
> - rtm_addrs |= RTA_NETMASK;
> - prefixlen(AF_INET6, plen);
> -
> - len = strtonum(plen, 0, 128, );
> - if (errstr)
> - errx(1, "prefixlen %s is %s", plen, errstr);
> -
> - q = (128-len) >> 3;
> - r = (128-len) & 7;
> - i = 15;
> -
> - while (q-- > 0)
> - sin6->sin6_addr.s6_addr[i--] = 0;
> - if (r > 0)
> - sin6->sin6_addr.s6_addr[i] &= 0xff << r;
> + rtm_addrs |= RTA_NETMASK;
Can't we consider this done in prefixlen() ?
>
> - return (0);
> - }
> + for (i = 0; i < 16; ++i)
> + sin6->sin6_addr.s6_addr[i] &= so_mask.sin6.sin6_addr.s6_addr[i];
> +
> + return (0);
> }
>
> /*
>
