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?

> 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.

> 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.

By the way 'softc' is generally for device drivers, you might prefer
something like bfdd (d for descriptor).

>  #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).

>               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?

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.

> +
> +     printf("\n");

I'd keep the pmsg_addrs() idiom, especially calling fflush(stdout).
>  }

> 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?

> +
> +     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.

> +
> +     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?

>  
>       /* 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.

>       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.

>       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() :)

Reply via email to