Convert bridge span & int. list to SLIST()
Using SLIST() instead of TAILQ() is a step towards using lock-less
lists.
As found in my previous attempts the interface list cannot be protected
by a mutex as long a if_enqueue() might grab the KERNEL_LOCK(). So I'm
heading towards using SRPL_*().
Ok?
Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.315
diff -u -p -r1.315 if_bridge.c
--- net/if_bridge.c 12 Dec 2018 14:19:15 - 1.315
+++ net/if_bridge.c 12 Dec 2018 14:27:21 -
@@ -108,6 +108,8 @@ voidbridgeattach(int);
intbridge_ioctl(struct ifnet *, u_long, caddr_t);
void bridge_ifdetach(void *);
void bridge_spandetach(void *);
+intbridge_ifremove(struct bridge_iflist *);
+intbridge_spanremove(struct bridge_iflist *);
intbridge_input(struct ifnet *, struct mbuf *, void *);
void bridge_process(struct ifnet *, struct mbuf *);
void bridgeintr_frame(struct bridge_softc *, struct ifnet *, struct mbuf *);
@@ -170,8 +172,8 @@ bridge_clone_create(struct if_clone *ifc
sc->sc_brtmax = BRIDGE_RTABLE_MAX;
sc->sc_brttimeout = BRIDGE_RTABLE_TIMEOUT;
timeout_set(>sc_brtimeout, bridge_rtage, sc);
- TAILQ_INIT(>sc_iflist);
- TAILQ_INIT(>sc_spanlist);
+ SLIST_INIT(>sc_iflist);
+ SLIST_INIT(>sc_spanlist);
for (i = 0; i < BRIDGE_RTABLE_SIZE; i++)
LIST_INIT(>sc_rts[i]);
arc4random_buf(>sc_hashkey, sizeof(sc->sc_hashkey));
@@ -216,10 +218,14 @@ bridge_clone_destroy(struct ifnet *ifp)
bridge_stop(sc);
bridge_rtflush(sc, IFBF_FLUSHALL);
- while ((bif = TAILQ_FIRST(>sc_iflist)) != NULL)
+ while ((bif = SLIST_FIRST(>sc_iflist)) != NULL) {
+ SLIST_REMOVE_HEAD(>sc_iflist, bif_next);
bridge_delete(sc, bif);
- while ((bif = TAILQ_FIRST(>sc_spanlist)) != NULL)
+ }
+ while ((bif = SLIST_FIRST(>sc_spanlist)) != NULL) {
+ SLIST_REMOVE_HEAD(>sc_spanlist, bif_next);
bridge_spandetach(bif);
+ }
bstp_destroy(sc->sc_stp);
@@ -249,7 +255,6 @@ bridge_delete(struct bridge_softc *sc, s
hook_disestablish(bif->ifp->if_detachhooks, bif->bif_dhcookie);
if_ih_remove(bif->ifp, bridge_input, NULL);
- TAILQ_REMOVE(>sc_iflist, bif, next);
bridge_rtdelete(sc, bif->ifp, 0);
bridge_flushrule(bif);
free(bif, M_DEVBUF, sizeof *bif);
@@ -297,7 +302,7 @@ bridge_ioctl(struct ifnet *ifp, u_long c
}
/* If it's in the span list, it can't be a member. */
- TAILQ_FOREACH(bif, >sc_spanlist, next) {
+ SLIST_FOREACH(bif, >sc_spanlist, bif_next) {
if (bif->ifp == ifs)
break;
}
@@ -338,7 +343,7 @@ bridge_ioctl(struct ifnet *ifp, u_long c
bif->bif_dhcookie = hook_establish(ifs->if_detachhooks, 0,
bridge_ifdetach, bif);
if_ih_insert(bif->ifp, bridge_input, NULL);
- TAILQ_INSERT_TAIL(>sc_iflist, bif, next);
+ SLIST_INSERT_HEAD(>sc_iflist, bif, bif_next);
break;
case SIOCBRDGDEL:
if ((error = suser(curproc)) != 0)
@@ -353,7 +358,7 @@ bridge_ioctl(struct ifnet *ifp, u_long c
error = ESRCH;
break;
}
- error = bridge_delete(sc, bif);
+ error = bridge_ifremove(bif);
break;
case SIOCBRDGIFS:
error = bridge_bifconf(sc, (struct ifbifconf *)data);
@@ -375,7 +380,7 @@ bridge_ioctl(struct ifnet *ifp, u_long c
error = EBUSY;
break;
}
- TAILQ_FOREACH(bif, >sc_spanlist, next) {
+ SLIST_FOREACH(bif, >sc_spanlist, bif_next) {
if (bif->ifp == ifs)
break;
}
@@ -395,7 +400,7 @@ bridge_ioctl(struct ifnet *ifp, u_long c
bridge_spandetach, bif);
SIMPLEQ_INIT(>bif_brlin);
SIMPLEQ_INIT(>bif_brlout);
- TAILQ_INSERT_TAIL(>sc_spanlist, bif, next);
+ SLIST_INSERT_HEAD(>sc_spanlist, bif, bif_next);
break;
case SIOCBRDGDELS:
if ((error = suser(curproc)) != 0)
@@ -405,15 +410,15 @@ bridge_ioctl(struct ifnet *ifp, u_long c
error = ENOENT;
break;
}
- TAILQ_FOREACH(bif, >sc_spanlist, next) {
+ SLIST_FOREACH(bif, >sc_spanlist, bif_next) {
if (bif->ifp == ifs)
break;
}
if (bif == NULL) {
- error = ENOENT;
+ error = ESRCH;
break;
}
-
Re: MPLSv6 1/2: kernel diff
On 18/12/18(Tue) 12:13, Denis Fondras wrote:
> Here is a serie of diffs to enable MPLSv6, MPLS transport over IPv6.
Nice!
> First diff : allow mpe(4) to handle IPv6 trafic.
Comments below
> Index: net/if_ethersubr.c
> ===
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.255
> diff -u -p -r1.255 if_ethersubr.c
> --- net/if_ethersubr.c12 Dec 2018 05:38:26 - 1.255
> +++ net/if_ethersubr.c18 Dec 2018 11:03:33 -
> @@ -246,18 +246,28 @@ ether_resolve(struct ifnet *ifp, struct
> sizeof(eh->ether_dhost));
> break;
> #ifdef INET6
> +do_v6:
> case AF_INET6:
> error = nd6_resolve(ifp, rt, m, dst, eh->ether_dhost);
> if (error)
> return (error);
> break;
> #endif
> +do_v4:
> case AF_INET:
> - case AF_MPLS:
> error = arpresolve(ifp, rt, m, dst, eh->ether_dhost);
> if (error)
> return (error);
> break;
> + case AF_MPLS:
> + switch (rt->rt_gateway->sa_family) {
> + case AF_INET:
> + goto do_v4;
> +#ifdef INET6
> + case AF_INET6:
> + goto do_v6;
> +#endif
> + }
> default:
> senderr(EHOSTUNREACH);
> }
This makes ether_resolve() (aka ether_output()) even more spaghetti for MPLS.
I know this is not your code, but is it possible to do better than that?
The current logic hides an encapsulation. For example would it be possible
to call ether_resolve() recursively in the AF_MPLS case?
> Index: net/if_mpe.c
> ===
> RCS file: /cvs/src/sys/net/if_mpe.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 if_mpe.c
> --- net/if_mpe.c 9 Jan 2018 15:24:24 - 1.64
> +++ net/if_mpe.c 18 Dec 2018 11:03:33 -
> @@ -85,7 +85,7 @@ mpe_clone_create(struct if_clone *ifc, i
> mpeif->sc_unit = unit;
> ifp = >sc_if;
> snprintf(ifp->if_xname, sizeof ifp->if_xname, "mpe%d", unit);
> - ifp->if_flags = IFF_POINTOPOINT;
> + ifp->if_flags = IFF_POINTOPOINT|IFF_MULTICAST;
You added this flag because the nd6 codes relies on it, right? But does
this reflect reality?
> ifp->if_xflags = IFXF_CLONED;
> ifp->if_softc = mpeif;
> ifp->if_mtu = MPE_MTU;
> @@ -157,6 +157,16 @@ mpestart(struct ifnet *ifp0)
> sizeof(in_addr_t));
> m_adj(m, sizeof(in_addr_t));
> break;
> +#ifdef INET6
> + case AF_INET6:
> + memset(sa, 0, sizeof(struct sockaddr_in6));
> + satosin6(sa)->sin6_family = af;
> + satosin6(sa)->sin6_len = sizeof(struct sockaddr_in6);
> + bcopy(mtod(m, caddr_t), (sa)->sin6_addr,
> + sizeof(struct in6_addr));
> + m_adj(m, sizeof(struct in6_addr));
> + break;
> +#endif
> default:
> m_freem(m);
> continue;
> @@ -204,6 +214,9 @@ mpeoutput(struct ifnet *ifp, struct mbuf
> int error;
> int off;
> in_addr_t addr;
> +#ifdef INET6
> + struct in6_addr addr6;
> +#endif
> u_int8_top = 0;
>
> #ifdef DIAGNOSTIC
> @@ -251,6 +264,39 @@ mpeoutput(struct ifnet *ifp, struct mbuf
> m_copyback(m, sizeof(sa_family_t), sizeof(in_addr_t),
> , M_NOWAIT);
> break;
> +#ifdef INET6
> + case AF_INET6:
> + if (!rt || !(rt->rt_flags & RTF_MPLS)) {
I know you copied past, but checking for rt != NULL should be avoided.
You should use rtisvalid(). Could you please merge the blocks related
to rtentry. I general if you can reduce the copy/paste between v4 & v6
that would be great!
Another style neat, we use the ISSET() macro for checking RTF_* flags ;)
> + m_freem(m);
> + error = ENETUNREACH;
> + goto out;
> + }
> + shim.shim_label = ((struct rt_mpls *)rt->rt_llinfo)->mpls_label;
> + shim.shim_label |= MPLS_BOS_MASK;
> + op = ((struct rt_mpls *)rt->rt_llinfo)->mpls_operation;
> + if (op != MPLS_OP_PUSH) {
> + m_freem(m);
> + error = ENETUNREACH;
> + goto out;
> + }
> + if (mpls_mapttl_ip) {
> + struct ip6_hdr *ip6;
> + ip6 = mtod(m, struct ip6_hdr *);
> + shim.shim_label |= htonl(ip6->ip6_hops) &
Re: Another attempt to bypass ifqs on interfaces
On 20/12/18(Thu) 11:35, David Gwynne wrote:
> My last attempt in "let etherip(4) output directly to the ip stack"
> didn't go so well. This one at least doesn't break bridge(4) on
> Ethernet.
It breaks it on mpw(4).
> This diff effectively turns if_enqueue into a per interface function
> pointer. Things still call if_enqueue(), which does some pf stuff, and then
> it calls ifp->if_enqueue. By default, that function pointer is
> if_enqueue_ifq() which puts the packet on the interface send queues.
>
> Ethernet interfaces intercept the call to if_enqueue_ifq(). They get set
> up to call ether_enqueue, which does the bridge stuff and then calls
> ac->ac_enqueue, which shadows the ifp->if_enqueue functionality.
Why do we need two abstraction layers? It feels weird to have
pseudo-drivers rely on a pointer in 'struct arpcom', because
they don't necessarily need this structure.
> This takes Ethernet bridge(4) knowledge out of the generic interface
> code. Non-Ethernet things that want to bypass ifqs can override
> ifp->if_enqueue. Ethernet things can override ac->ac_enqueue.
Why take it out of the generic? We spent a lot of energy putting it
there, now you're taking the opposite direction. So let's spread
NBRIDGE > 1 all over the stack again?
> This diff includes a change to vlan(4) to take advantage of this. It
> overrides ac_enqueue with vlan_enqueue(). So a packet sent out a vlan
> interface will go into if_enqueue(), which will call ether_enqueue()
> because ether_ifattach set that up. ether_enqueue() will call
> ac->ac_enqueue which is vlan_enqueue on vlan interfaces, which will
> eventually call if_enqueue() on the parent interface.
I like the idea behind vlan_transmit().
>
> There are some things I'd like to try if this goes in. eg, it may be
> possible to have ether_enqueue simple call ac->ac_enqueue, and let
> bridge(4) swap it out with bridge_enqueue code. Doing that now would
> complicate the diff though.
Why? What's the gain?
> Anyway, how's this look, esp relative to the previous diff?
It looks better but I'm afraid this diff still breaks stuff, I'd
suggest you to keep your fix simple. I'm afraid with all the
abstractions that you're introducing. They look like premature
optimizations to me.
One more comment below.
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.569
> diff -u -p -r1.569 if.c
> --- net/if.c 11 Dec 2018 22:08:57 - 1.569
> +++ net/if.c 19 Dec 2018 06:48:37 -
> @@ -683,34 +685,33 @@ if_qstart_compat(struct ifqueue *ifq)
> int
> if_enqueue(struct ifnet *ifp, struct mbuf *m)
> {
> - unsigned int idx;
> - struct ifqueue *ifq;
> - int error;
> -
> #if NPF > 0
> if (m->m_pkthdr.pf.delay > 0)
> return (pf_delay_pkt(m, ifp->if_index));
> -#endif
> -
> -#if NBRIDGE > 0
> - if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0) {
> - KERNEL_LOCK();
> - error = bridge_output(ifp, m, NULL, NULL);
> - KERNEL_UNLOCK();
> - return (error);
> - }
> -#endif
>
> -#if NPF > 0
> pf_pkt_addr_changed(m);
> #endif /* NPF > 0 */
Are you sure calling pf_pkt_addr_changed() before bridge_output() won't
create any unwanted side effect? In any case I'd suggest keeping this
separated, to be able to revert it easily :D
Re: mg: Delete region
Hi Hiltjo, Hiltjo Posthuma wrote on Sun, Dec 23, 2018 at 03:34:12PM +0100: > Shouldn't these kind of diffs be OK'd by other developers before > committing though? Not necessarily. When a developer focusses on work in a particular area and takes responsibility to fix anything that breaks there, in particular when few or no others work in the area, that developer owns that code. When few others work in an area, requiring the code owner to get OKs for each and every improvement can be detrimental because on the one hand, it can slow down development for little benefit, and on the other hand, it can put undue strees on the few others who occasionally look at that code, too. Both getting and giving OKs can be frustrating and time-consuming. Espie doesn't ask for OKs in pkg_add(1), not even for major changes, unless he is worried whether or not some general direction is desired. Neither do i in mandoc(1). Neither does landry@ in firefox nor robert@ in chrome. And so on. Even though i don't use mg(1), my impression is that lum@ owns that code nowadays, that almost the only other developer occasionally contributing to it is florian@, and that these two have been doing a very decent job during the last few years: i haven't noticed any complaints about their work on mg(1). Unless other *developers* request a backout, the code owner gets to decide about the details of the way forward. And "the current behaviour is already better than what was there last week, and i'll work on it next week for further improvements" is definitely an adequate response. Yours, Ingo
Re: mandoc -T html default style
On Fri, 21 Dec 2018 00:10:10 +0100, Ingo Schwarze wrote: > I see the point, and it makes sense to me. > > So in case people want a sensible default for "-O style=" as suggested, > where should the file go? > > Maybe /usr/share/misc/mandoc.css ? That would be fine with me. I definitely think it should be installed somewhere. I found it odd that I had to look for mandoc.css in the source tree when I needed it. - todd - todd
Re: mg: Delete region
On Sun, Dec 23, 2018 at 01:58:16PM +, Mark Lumsden wrote: > On Sun, 23 Dec 2018, Hiltjo Posthuma wrote: > > > Date: Sun, 23 Dec 2018 13:43:32 +0100 > > From: Hiltjo Posthuma > > To: Mark Lumsden > > Cc: Leonid Bobrov , tech@openbsd.org > > Subject: Re: mg: Delete region > > > > On Thu, Dec 20, 2018 at 06:44:15AM +, Mark Lumsden wrote: > > > hmm, you are correct. I'm trying to remember which machine I tested > > > on that made me come to that conclusion. > > > > > > On Thu, 20 Dec 2018, Leonid Bobrov wrote: > > > > > > > Date: Thu, 20 Dec 2018 06:43:02 +0200 > > > > From: Leonid Bobrov > > > > To: Mark Lumsden , tech@openbsd.org > > > > Subject: Re: mg: Delete region > > > > > > > > I tried this in GNU Emacs and it didn't place the killed region into the > > > > kill buffer. It only places it into the kill buffer if you pressed C-w > > > > > > > > > > > Hi, > > > > Can this change get reverted upstream until it is properly tested (and > > OK'd), > > please? > > > > -- > > Kind regards, > > Hiltjo > > > > Its should be trivial to make it behave as emacs does. I should get time in > about a week to have a look. Personally I don't think it needs reverted > since it is more useful having it behave as it does just now than > previously. But if someone wants to do so they are free to do that. Shouldn't these kind of diffs be OK'd by other developers before committing though? Backspace should just delete a character backwards and not have some context-specific behaviour. A person can just bind kill-region to some key if deletion is wanted. This should not be in the C code. I reverted the change locally. I use marked regions with other actions all the time and this change makes mg unusable for me. -- Kind regards, Hiltjo
Re: mg: Delete region
On Sun, 23 Dec 2018, Hiltjo Posthuma wrote: Date: Sun, 23 Dec 2018 13:43:32 +0100 From: Hiltjo Posthuma To: Mark Lumsden Cc: Leonid Bobrov , tech@openbsd.org Subject: Re: mg: Delete region On Thu, Dec 20, 2018 at 06:44:15AM +, Mark Lumsden wrote: hmm, you are correct. I'm trying to remember which machine I tested on that made me come to that conclusion. On Thu, 20 Dec 2018, Leonid Bobrov wrote: Date: Thu, 20 Dec 2018 06:43:02 +0200 From: Leonid Bobrov To: Mark Lumsden , tech@openbsd.org Subject: Re: mg: Delete region I tried this in GNU Emacs and it didn't place the killed region into the kill buffer. It only places it into the kill buffer if you pressed C-w Hi, Can this change get reverted upstream until it is properly tested (and OK'd), please? -- Kind regards, Hiltjo Its should be trivial to make it behave as emacs does. I should get time in about a week to have a look. Personally I don't think it needs reverted since it is more useful having it behave as it does just now than previously. But if someone wants to do so they are free to do that.
Re: mg: Delete region
On Thu, Dec 20, 2018 at 06:44:15AM +, Mark Lumsden wrote: > hmm, you are correct. I'm trying to remember which machine I tested > on that made me come to that conclusion. > > On Thu, 20 Dec 2018, Leonid Bobrov wrote: > > > Date: Thu, 20 Dec 2018 06:43:02 +0200 > > From: Leonid Bobrov > > To: Mark Lumsden , tech@openbsd.org > > Subject: Re: mg: Delete region > > > > I tried this in GNU Emacs and it didn't place the killed region into the > > kill buffer. It only places it into the kill buffer if you pressed C-w > > > Hi, Can this change get reverted upstream until it is properly tested (and OK'd), please? -- Kind regards, Hiltjo
Re: MPLS pseudowire over IPv6 1/2: ifconfig(8) diff
On Sun, Dec 23, 2018 at 10:05:01AM +, Jason McIntyre wrote:
> unless you're trying somehow to emphasise that both types of address are
> supported, you could simplify the text to just:
>
> The
> .Ar dest-address
> is used to find ...
>
Thank you for your input.
Index: ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.324
diff -u -p -r1.324 ifconfig.8
--- ifconfig.8 16 Nov 2018 12:25:29 - 1.324
+++ ifconfig.8 23 Dec 2018 10:22:56 -
@@ -1344,8 +1344,7 @@ is another 20-bit number which will be u
Sets the destination address where this mpw should output.
The
.Ar dest-address
-is an IPv4 address that will be used to find the nexthop in the MPLS
-network.
+is used to find the nexthop in the MPLS network.
.El
.Sh PAIR
.nr nS 1
Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.387
diff -u -p -r1.387 ifconfig.c
--- ifconfig.c 29 Nov 2018 00:12:34 - 1.387
+++ ifconfig.c 23 Dec 2018 10:22:57 -
@@ -3802,8 +3802,9 @@ mpe_status(void)
void
mpw_status(void)
{
- struct sockaddr_in *sin;
- struct ifmpwreq imr;
+ struct sockaddr_storage *ss;
+ struct ifmpwreq imr;
+ char hbuf[NI_MAXHOST];
bzero(, sizeof(imr));
ifr.ifr_data = (caddr_t)
@@ -3842,11 +3843,19 @@ mpw_status(void)
else
printf("remote %u\n", imr.imr_rshim.shim_label);
- sin = (struct sockaddr_in *) _nexthop;
- if (sin->sin_addr.s_addr == 0)
- printf("\tneighbor: none\n");
- else
- printf("\tneighbor: %s\n", inet_ntoa(sin->sin_addr));
+ printf("\tneighbor: ");
+ switch (imr.imr_nexthop.ss_family) {
+ case AF_INET6:
+ case AF_INET:
+ ss = (struct sockaddr_storage *) _nexthop;
+ if (getnameinfo((struct sockaddr *)ss, ss->ss_len, hbuf,
+ sizeof(hbuf), NULL, 0, NI_NUMERICHOST) != 0)
+ strlcpy(hbuf, "???", sizeof(hbuf));
+ printf("%s\n", hbuf);
+ break;
+ default:
+ printf("none\n");
+ }
}
/* ARGSUSED */
@@ -3870,6 +3879,7 @@ void
process_mpw_commands(void)
{
struct sockaddr_in *sin, *sinn;
+ struct sockaddr_in6 *sin6, *sin6n;
struct ifmpwreq imr;
if (wconfig == 0)
@@ -3899,14 +3909,29 @@ process_mpw_commands(void)
imrsave.imr_rshim.shim_label = imr.imr_rshim.shim_label;
}
- sin = (struct sockaddr_in *) _nexthop;
- sinn = (struct sockaddr_in *) _nexthop;
- if (sin->sin_addr.s_addr == 0) {
- if (sinn->sin_addr.s_addr == 0)
+ switch (imrsave.imr_nexthop.ss_family) {
+ case AF_INET6:
+ case AF_INET:
+ /* neighbor was set earlier, nothing to do */
+ break;
+ default:
+ switch (imr.imr_nexthop.ss_family) {
+ case AF_INET6:
+ sin6 = (struct sockaddr_in6 *) _nexthop;
+ sin6n = (struct sockaddr_in6 *) _nexthop;
+ sin6->sin6_family = sin6n->sin6_family;
+ memcpy(>sin6_addr, >sin6_addr,
+ sizeof(struct in6_addr));
+ break;
+ case AF_INET:
+ sin = (struct sockaddr_in *) _nexthop;
+ sinn = (struct sockaddr_in *) _nexthop;
+ sin->sin_family = sinn->sin_family;
+ sin->sin_addr.s_addr = sinn->sin_addr.s_addr;
+ break;
+ default:
errx(1, "mpw neighbor address not specified");
-
- sin->sin_family = sinn->sin_family;
- sin->sin_addr.s_addr = sinn->sin_addr.s_addr;
+ }
}
ifr.ifr_data = (caddr_t)
@@ -3949,14 +3974,41 @@ void
setmpwneighbor(const char *value, int d)
{
struct sockaddr_in *sin;
+ struct sockaddr_in6 *sin6;
+ struct addrinfo hints, *res;
+ int rv;
wconfig = 1;
- sin = (struct sockaddr_in *) _nexthop;
- if (inet_aton(value, >sin_addr) == 0)
+ memset(, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_DGRAM;
+ hints.ai_protocol = 0;
+ hints.ai_flags = AI_PASSIVE;
+
+ rv = getaddrinfo(value, NULL, , );
+ if (rv != 0)
+ errx(1, "neighbor %s: %s", value, gai_strerror(rv));
+
+ switch (res->ai_family) {
+ case AF_INET:
+ sin = (struct sockaddr_in *) _nexthop;
+ sin->sin_family = res->ai_family;
+ sin->sin_addr.s_addr =
+ ((struct sockaddr_in *)res->ai_addr)->sin_addr.s_addr;
+ break;
+
Re: MPLS pseudowire over IPv6 1/2: ifconfig(8) diff
On Sun, Dec 23, 2018 at 10:28:30AM +0100, Denis Fondras wrote: > On Sat, Dec 22, 2018 at 11:42:36PM +0100, Klemens Nanni wrote: > > I have no clue about MPLS, so this feedback is code-wise only: > > > > Did you consider using more AF agnostic code for parsing the IP address? > > > > You are right, it is simpler. Thank you for the clue :) > > Updated diff : > > Index: ifconfig.8 > === > RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v > retrieving revision 1.324 > diff -u -p -r1.324 ifconfig.8 > --- ifconfig.816 Nov 2018 12:25:29 - 1.324 > +++ ifconfig.823 Dec 2018 09:25:36 - > @@ -1344,7 +1344,7 @@ is another 20-bit number which will be u > Sets the destination address where this mpw should output. > The > .Ar dest-address > -is an IPv4 address that will be used to find the nexthop in the MPLS > +is an IPv4 or IPv6 address that will be used to find the nexthop in the MPLS > network. > .El > .Sh PAIR morning. unless you're trying somehow to emphasise that both types of address are supported, you could simplify the text to just: The .Ar dest-address is used to find ... jmc
Re: MPLS pseudowire over IPv6 1/2: ifconfig(8) diff
On Sat, Dec 22, 2018 at 11:42:36PM +0100, Klemens Nanni wrote:
> I have no clue about MPLS, so this feedback is code-wise only:
>
> Did you consider using more AF agnostic code for parsing the IP address?
>
You are right, it is simpler. Thank you for the clue :)
Updated diff :
Index: ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.324
diff -u -p -r1.324 ifconfig.8
--- ifconfig.8 16 Nov 2018 12:25:29 - 1.324
+++ ifconfig.8 23 Dec 2018 09:25:36 -
@@ -1344,7 +1344,7 @@ is another 20-bit number which will be u
Sets the destination address where this mpw should output.
The
.Ar dest-address
-is an IPv4 address that will be used to find the nexthop in the MPLS
+is an IPv4 or IPv6 address that will be used to find the nexthop in the MPLS
network.
.El
.Sh PAIR
Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.387
diff -u -p -r1.387 ifconfig.c
--- ifconfig.c 29 Nov 2018 00:12:34 - 1.387
+++ ifconfig.c 23 Dec 2018 09:25:36 -
@@ -3802,8 +3802,9 @@ mpe_status(void)
void
mpw_status(void)
{
- struct sockaddr_in *sin;
- struct ifmpwreq imr;
+ struct sockaddr_storage *ss;
+ struct ifmpwreq imr;
+ char hbuf[NI_MAXHOST];
bzero(, sizeof(imr));
ifr.ifr_data = (caddr_t)
@@ -3842,11 +3843,19 @@ mpw_status(void)
else
printf("remote %u\n", imr.imr_rshim.shim_label);
- sin = (struct sockaddr_in *) _nexthop;
- if (sin->sin_addr.s_addr == 0)
- printf("\tneighbor: none\n");
- else
- printf("\tneighbor: %s\n", inet_ntoa(sin->sin_addr));
+ printf("\tneighbor: ");
+ switch (imr.imr_nexthop.ss_family) {
+ case AF_INET6:
+ case AF_INET:
+ ss = (struct sockaddr_storage *) _nexthop;
+ if (getnameinfo((struct sockaddr *)ss, ss->ss_len, hbuf,
+ sizeof(hbuf), NULL, 0, NI_NUMERICHOST) != 0)
+ strlcpy(hbuf, "???", sizeof(hbuf));
+ printf("%s\n", hbuf);
+ break;
+ default:
+ printf("none\n");
+ }
}
/* ARGSUSED */
@@ -3870,6 +3879,7 @@ void
process_mpw_commands(void)
{
struct sockaddr_in *sin, *sinn;
+ struct sockaddr_in6 *sin6, *sin6n;
struct ifmpwreq imr;
if (wconfig == 0)
@@ -3899,14 +3909,29 @@ process_mpw_commands(void)
imrsave.imr_rshim.shim_label = imr.imr_rshim.shim_label;
}
- sin = (struct sockaddr_in *) _nexthop;
- sinn = (struct sockaddr_in *) _nexthop;
- if (sin->sin_addr.s_addr == 0) {
- if (sinn->sin_addr.s_addr == 0)
+ switch (imrsave.imr_nexthop.ss_family) {
+ case AF_INET6:
+ case AF_INET:
+ /* neighbor was set earlier, nothing to do */
+ break;
+ default:
+ switch (imr.imr_nexthop.ss_family) {
+ case AF_INET6:
+ sin6 = (struct sockaddr_in6 *) _nexthop;
+ sin6n = (struct sockaddr_in6 *) _nexthop;
+ sin6->sin6_family = sin6n->sin6_family;
+ memcpy(>sin6_addr, >sin6_addr,
+ sizeof(struct in6_addr));
+ break;
+ case AF_INET:
+ sin = (struct sockaddr_in *) _nexthop;
+ sinn = (struct sockaddr_in *) _nexthop;
+ sin->sin_family = sinn->sin_family;
+ sin->sin_addr.s_addr = sinn->sin_addr.s_addr;
+ break;
+ default:
errx(1, "mpw neighbor address not specified");
-
- sin->sin_family = sinn->sin_family;
- sin->sin_addr.s_addr = sinn->sin_addr.s_addr;
+ }
}
ifr.ifr_data = (caddr_t)
@@ -3949,14 +3974,41 @@ void
setmpwneighbor(const char *value, int d)
{
struct sockaddr_in *sin;
+ struct sockaddr_in6 *sin6;
+ struct addrinfo hints, *res;
+ int rv;
wconfig = 1;
- sin = (struct sockaddr_in *) _nexthop;
- if (inet_aton(value, >sin_addr) == 0)
+ memset(, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_DGRAM;
+ hints.ai_protocol = 0;
+ hints.ai_flags = AI_PASSIVE;
+
+ rv = getaddrinfo(value, NULL, , );
+ if (rv != 0)
+ errx(1, "neighbor %s: %s", value, gai_strerror(rv));
+
+ switch (res->ai_family) {
+ case AF_INET:
+ sin = (struct sockaddr_in *) _nexthop;
+ sin->sin_family = res->ai_family;
+ sin->sin_addr.s_addr =
+ ((struct sockaddr_in *)res->ai_addr)->sin_addr.s_addr;
+
