On 2016 Sep 15 (Thu) at 13:28:10 +0200 (+0200), Martin Pieuchot wrote:
:On 14/09/16(Wed) 15:50, Peter Hessler wrote:
:> This is a work-in-progress diff that I would like to commit. I can print
:> a few things, but there is a problem when trying to bring in more
:> fields. Printing is also ugly, but I can fix that in-tree.
:
:Well you're mixing multiples things in one diff, could you keep one
:topic per diff?
:
Yes, sorry about that.
:> While here, I print the descr's as ints, the same way Juniper does it.
:
:I'm not sure which part of the diff I should look at.
:
Ignore this part. I will take care of that seperately.
:> I also had to add RTM_INVALIDATE, to keep the ordering correct.
:
:This change is ok mpi@.
:
:> Am I tying this into route(8) and rtsock.c correctly?
:
:I think so, more comments below.
:
:> Index: sbin/route/route.c
:> ===================================================================
:> RCS file: /cvs/openbsd/src/sbin/route/route.c,v
:> retrieving revision 1.190
:> diff -u -p -u -p -r1.190 route.c
:> --- sbin/route/route.c 4 Sep 2016 09:41:03 -0000 1.190
:> +++ sbin/route/route.c 14 Sep 2016 10:20:04 -0000
:> @@ -41,6 +41,10 @@
:> #include <netinet/in.h>
:> #include <netmpls/mpls.h>
:>
:> +#include <sys/task.h>
:> +#include <sys/timeout.h>
:> +#include <net/bfd.h>
:
:You should guard kernel-only structures inside "#ifdef _KERNEL", no need
:to export bfd_softc to userland. This will prevent you from polluting
:the namespace an need to include kernel headers.
:
OK, will do.
:By the way 'softc' is generally for device drivers, you might prefer
:something like bfdd (d for descriptor).
There's be a whole lot of bikeshedding about this topic, I would prefer
not to change it for now.
:> #include <arpa/inet.h>
:> #include <netdb.h>
:>
:> @@ -90,6 +94,7 @@ void sodump(sup, char *);
:> char *priorityname(uint8_t);
:> uint8_t getpriority(char *);
:> void print_getmsg(struct rt_msghdr *, int);
:> +void print_bfdmsg(struct bfd_msghdr *);
:> const char *get_linkstate(int, int);
:> void print_rtmsg(struct rt_msghdr *, int);
:> void pmsg_common(struct rt_msghdr *);
:> @@ -1240,6 +1245,7 @@ char *msgtypes[] = {
:> "RTM_IFINFO: iface status change",
:> "RTM_IFANNOUNCE: iface arrival/departure",
:> "RTM_DESYNC: route socket overflow",
:> + "RTM_INVALIDATE: invalidate cache of L2 route",
:> "RTM_BFD: bidirectional forwarding detection",
:> };
:>
:> @@ -1277,6 +1283,7 @@ print_rtmsg(struct rt_msghdr *rtm, int m
:> struct if_msghdr *ifm;
:> struct ifa_msghdr *ifam;
:> struct if_announcemsghdr *ifan;
:> + struct bfd_msghdr *bfd;
:> char ifname[IF_NAMESIZE];
:>
:> if (verbose == 0)
:> @@ -1333,7 +1340,9 @@ print_rtmsg(struct rt_msghdr *rtm, int m
:> printf("\n");
:> break;
:> case RTM_BFD:
:> - printf("bfd\n"); /* XXX - expand*/
:> + bfd = (struct bfd_msghdr *)rtm;
:> + printf(", BFD ");
:> + print_bfdmsg(bfd);
:
:I wonder if the BFD code shouldn't be under #ifdef SMALL, you should
:check with deraadt@ if it grows the size of route(8).
:
I'm happy to add that, sure. The kernel side should not be compiled
under SMALL either.
:> break;
:> default:
:> printf(", priority %d, table %u, ifidx %u, ",
:> @@ -1524,6 +1533,48 @@ print_getmsg(struct rt_msghdr *rtm, int
:> putchar('\n');
:> }
:> #undef RTA_IGN
:> +}
:> +
:> +void
:> +print_bfdmsg(struct bfd_msghdr *bfd)
:> +{
:> + printf("mode ");
:> + switch (bfd->mode) {
:> + case BFD_MODE_ASYNC:
:> + printf("ASYNC");
:> + break;
:> + case BFD_MODE_DEMAND:
:> + printf("DEMAND");
:> + break;
:> + }
:> + printf(" state ");
:> + switch (bfd->state) {
:> + case BFD_STATE_ADMINDOWN:
:> + printf("AdminDown");
:> + break;
:> + case BFD_STATE_DOWN:
:> + printf("Down");
:> + break;
:> + case BFD_STATE_INIT:
:> + printf("Init");
:> + break;
:> + case BFD_STATE_UP:
:> + printf("Up");
:> + break;
:> + }
:> + printf(" error %d", bfd->error);
:> + printf(" localdiscr %u", bfd->localdiscr);
:> + printf(" remotediscr %u", bfd->remotediscr);
:> + printf(" localdiag %u", bfd->localdiag);
:> + printf(" remotediag %u", bfd->remotediag);
:> + printf(" uptime %lld", bfd->uptime);
:> + printf(" lastuptime %lld", bfd->lastuptime);
:> +
:> + printf(" mintx %ums", bfd->mintx / 1000);
:> + printf(" minrx %ums", bfd->minrx / 1000);
:> + printf(" multiplier %ux", bfd->multiplier);
:
:In this output you're mixing UPERCASE, camelCase and lowercase, is it on
:purpose?
:
Partially on purpose. This part will get a lot more fixes to make it
easier to read and to deal with.
I can lowercase everything, that is fine.
:Another remark, could you prefix the field name of your "struct bfd_msghdr"
:with ``bfdm_''? This would make your code coherent with the existing
:if_msghr, ifa_msghr, if_announcemsghdr and rt_msghdr.
:
Sure.
:> +
:> + printf("\n");
:
:I'd keep the pmsg_addrs() idiom, especially calling fflush(stdout).
Ok, I'll take a look at that.
:> }
:
:> Index: sys/net/bfd.c
:> ===================================================================
:> RCS file: /cvs/openbsd/src/sys/net/bfd.c,v
:> retrieving revision 1.24
:> diff -u -p -u -p -r1.24 bfd.c
:> --- sys/net/bfd.c 13 Sep 2016 07:56:05 -0000 1.24
:> +++ sys/net/bfd.c 14 Sep 2016 10:48:39 -0000
:> @@ -161,7 +161,7 @@ struct bfd_state {
:> uint32_t AuthSeqKnown;
:> };
:>
:> -struct pool bfd_pool, bfd_pool_peer, bfd_pool_time;
:> +struct pool bfd_pool, bfd_pool_peer, bfd_pool_time, bfd_pool_msghdr;
:> struct taskq *bfdtq;
:>
:> struct socket *bfd_listener(struct bfd_softc *, unsigned int);
:> @@ -182,6 +182,7 @@ void bfd_senddown(struct bfd_softc *);
:> void bfd_reset(struct bfd_softc *);
:> void bfd_set_uptime(struct bfd_softc *);
:>
:> +void bfd_prepmsg(struct bfd_softc *);
:> void bfd_debug(struct bfd_softc *);
:>
:> TAILQ_HEAD(bfd_queue, bfd_softc) bfd_queue;
:> @@ -224,9 +225,38 @@ bfd_rtalloc(struct rtentry *rt)
:>
:> TAILQ_INSERT_TAIL(&bfd_queue, sc, bfd_next);
:>
:> + bfd_prepmsg(sc);
:> return (0);
:> }
:>
:> +void
:> +bfd_prepmsg(struct bfd_softc *sc)
:> +{
:> + struct bfd_msghdr *bfd;
: ^^^
:
:What about using ``bfdm'' to make it clear it's a message?
:
Sure.
:> +
:> + bfd = pool_get(&bfd_pool_msghdr, PR_WAITOK | PR_ZERO);
:
:This doesn't make sense you're allocating memory just to pass
:arguments to the rtsock layer. I believe this functions is
:not needed.
:
I ran into some problems with that, but I can take another look.
:> +
:> + bfd->mode = sc->mode;
:> + bfd->mintx = sc->mintx;
:> + bfd->minrx = sc->minrx;
:> + bfd->multiplier = sc->multiplier;
:> +
:> + bfd->uptime = sc->sc_time->tv_sec;
:> + bfd->lastuptime = sc->lastuptime;
:> + bfd->state = sc->state;
:> + bfd->laststate = sc->laststate;
:> + bfd->error = sc->error;
:> +
:> + bfd->localdiscr = sc->sc_peer->LocalDiscr;
:> + bfd->localdiag = sc->sc_peer->LocalDiag;
:> + bfd->remotediscr = sc->sc_peer->RemoteDiscr;
:> + bfd->remotediag = sc->sc_peer->RemoteDiag;
:> +
:> + bfd_msg(bfd);
:> +
:> + pool_put(&bfd_pool, bfd);
:> +}
:> +
:> /*
:> * remove and free a bfd session
:> */
:> @@ -283,6 +313,9 @@ bfdinit(void)
:> pool_init(&bfd_pool_time, sizeof(struct timeval), 0, 0, 0,
:> "bfd_softc_time", NULL);
:> pool_setipl(&bfd_pool_time, IPL_SOFTNET);
:> + pool_init(&bfd_pool_msghdr, sizeof(struct bfd_msghdr), 0, 0, 0,
:> + "bfd_msghdr", NULL);
:> + pool_setipl(&bfd_pool_msghdr, IPL_SOFTNET);
:>
:> bfdtq = taskq_create("bfd", 1, IPL_SOFTNET, 0);
:> if (bfdtq == NULL)
:> @@ -306,6 +339,7 @@ bfddestroy(void)
:> }
:>
:> taskq_destroy(bfdtq);
:> + pool_destroy(&bfd_pool_msghdr);
:> pool_destroy(&bfd_pool_time);
:> pool_destroy(&bfd_pool_peer);
:> pool_destroy(&bfd_pool);
:> @@ -376,6 +410,7 @@ bfd_send_task(void *arg)
:> bfd_set_state(sc, BFD_STATE_DOWN);
:> }
:> }
:> +bfd_prepmsg(sc);
:
:Why not call it bfd_sendmsg() and do everything you need there?
:
Because either bfd.c needs externs to use some globals in rtsock.c, or
rtsock.c needs all of the child structures from bfd.c.
I can look at this, as mentioned above.
:>
:> /* re-add 70%-90% jitter to our transmits, rfc 5880 6.8.7 */
:> timeout_add_usec(&sc->sc_timo_tx,
:> @@ -873,7 +908,7 @@ bfd_set_state(struct bfd_softc *sc, int
:> }
:>
:> printf("%s: BFD set linkstate %u (oldstate: %u)\n", ifp->if_xname,
new_state, state);
:> - rt_sendmsg(rt, new_state, ifp->if_rdomain);
:> + bfd_prepmsg(sc);
:>
:> if_put(ifp);
:>
:> @@ -970,8 +1005,8 @@ bfd_debug(struct bfd_softc *sc)
:> printf("local diag: %u ", sc->sc_peer->LocalDiag);
:> printf("\n");
:> printf("\t");
:> - printf("remote discriminator: 0x%x ", sc->sc_peer->RemoteDiscr);
:> - printf("local discriminator: 0x%x ", sc->sc_peer->LocalDiscr);
:> + printf("remote discriminator: %u ", sc->sc_peer->RemoteDiscr);
:> + printf("local discriminator: %u ", sc->sc_peer->LocalDiscr);
:
:Could you please get rid of these CamelCase names, that's not how we
:pick name, and we already follow many RFCs without (too much) confusion.
:
:Generally speaking when software tries to map 1:1 a RFC, the result is a
:very convoluted design.
:
The names in sc_peer are specific to the RFC and are heavily used in the
state table and in the various transition explanations. Unfortunately,
there are extremely similar names that are not only different things,
but also used in the same sections in the RFCs. I've named them like
this to make it easier to understand what the RFC is actually saying.
FWIW, sc_peer is the only structure where I want to do this.
Since this is part of the Juniper thing I mentioned above, we can ignore
this part for now.
:> printf("\n");
:> printf("\t");
:> printf("remote session state: %u ", sc->sc_peer->RemoteSessionState);
:> Index: sys/net/route.h
:> ===================================================================
:> RCS file: /cvs/openbsd/src/sys/net/route.h,v
:> retrieving revision 1.147
:> diff -u -p -u -p -r1.147 route.h
:> --- sys/net/route.h 4 Sep 2016 10:32:01 -0000 1.147
:> +++ sys/net/route.h 14 Sep 2016 09:09:29 -0000
:> @@ -356,6 +356,7 @@ struct mbuf;
:> struct socket;
:> struct ifnet;
:> struct sockaddr_in6;
:> +struct bfd_msghdr;
:>
:> void route_init(void);
:> int route_output(struct mbuf *, ...);
:> @@ -363,6 +364,7 @@ int route_usrreq(struct socket *, int,
:> struct mbuf *, struct mbuf *, struct proc *);
:> void rt_ifmsg(struct ifnet *);
:> void rt_ifannouncemsg(struct ifnet *, int);
:> +void bfd_msg(struct bfd_msghdr *);
:
:I'd keep bfd_sendmsg() inside bfd.c so you don't need that.
:
:> void rt_maskedcopy(struct sockaddr *,
:> struct sockaddr *, struct sockaddr *);
:> struct sockaddr *rt_plen2mask(struct rtentry *, struct sockaddr_in6 *);
:> Index: sys/net/rtsock.c
:> ===================================================================
:> RCS file: /cvs/openbsd/src/sys/net/rtsock.c,v
:> retrieving revision 1.204
:> diff -u -p -u -p -r1.204 rtsock.c
:> --- sys/net/rtsock.c 7 Sep 2016 09:36:49 -0000 1.204
:> +++ sys/net/rtsock.c 14 Sep 2016 13:37:04 -0000
:> @@ -1100,6 +1100,9 @@ rt_msg1(int type, struct rt_addrinfo *rt
:> case RTM_IFANNOUNCE:
:> len = sizeof(struct if_announcemsghdr);
:> break;
:> + case RTM_BFD:
:> + len = sizeof(struct bfd_msghdr);
:> + break;
:
:This is correct, I'd keep #ifdef BFD at least for coherency.
:
Added locally.
:> default:
:> len = sizeof(struct rt_msghdr);
:> break;
:> @@ -1329,6 +1332,27 @@ rt_ifannouncemsg(struct ifnet *ifp, int
:> ifan->ifan_index = ifp->if_index;
:> strlcpy(ifan->ifan_name, ifp->if_xname, sizeof(ifan->ifan_name));
:> ifan->ifan_what = what;
:> + route_proto.sp_protocol = 0;
:> + route_input(m, &route_proto, &route_src, &route_dst);
:> +}
:> +
:> +void
:> +bfd_msg(struct bfd_msghdr *bfd0)
:> +{
:> + struct bfd_msghdr *bfd;
:> + struct mbuf *m;
:> +
:> + if (route_cb.any_count == 0)
:> + return;
:> + m = rt_msg1(RTM_BFD, NULL);
:> + if (m == NULL)
:> + return;
:> + bfd = mtod(m, struct bfd_msghdr *);
:> +
:> + bfd->mode = bfd0->mode;
:> + bfd->localdiscr = bfd0->localdiscr;
:> + bfd->state = bfd0->state;
:> +
:> route_proto.sp_protocol = 0;
:> route_input(m, &route_proto, &route_src, &route_dst);
:
:This should be bfd_sendmsg() :)
Thanks for the review!
--
A university is what a college becomes when the faculty loses interest
in students.
-- John Ciardi
