Martin - Any word on this? I'd like to push this patch if it fixes your issues.
donald On Thu, Feb 18, 2016 at 10:30 PM, Martin Winter < [email protected]> wrote: > On 18 Feb 2016, at 18:35, Donald Sharp wrote: > > > Martin - > > > > rt_socket.c is not compiled on linux so no need for verification. Once > you > > have a run please let me know and I'll push your updated patch. > > It’s running now. Will know soon. It definitely fixes some of the errors > and I hope it actually fixes all of the FreeBSD errors. > > Will know in approx one more day. > > (BTW: Testing on a git commit which does no longer exist because of rebase, > but I needed the same to compare. Lucky that I still had the VMs with the > old git checkout running…) > > - Martin > > > > > donald > > > > On Thu, Feb 18, 2016 at 9:19 PM, Martin Winter < > > [email protected]> wrote: > > > >> Timo, > >> > >> On 17 Feb 2016, at 23:01, Timo Teras wrote: > >> > >> On Wed, 17 Feb 2016 20:11:07 -0800 > >>> "Martin Winter" <[email protected]> wrote: > >>> > >>> This is based on Quagga proposed 6 branch of Feb 10 (git f48d5b9957 - > >>>> which does no longer exist, [no] thanks to rebase on a public > >>>> git) on FreeBSD 10.2. > >>>> > >>>> Zebra seems to fail delete any routes in FreeBSD RIB. It fails with > >>>> updates (better routes to different interface) and > >>>> it fails to remove them when quagga is killed. > >>>> > >>> > >>> Thanks for the testing. Sounds like this is breakage caused by my > >>> atomic FIB patch which was pretty untested on *BSD. > >>> > >>> Looks like the code talking to kernel does not handle RTM_CHANGE > >>> correctly. As first test, perhaps just removing RTM_CHANGE usage might > >>> fix the issues and revert to how it used to work. > >>> > >>> Using RTM_CHANGE on kernels where it works is better, but it's left an > >>> exercise for developer who has access and will to fix it on *BSD. > >>> > >> > >> Thanks for the patch. Seems like you never tested it (failed to > compile), > >> but was close enough to guess what you probably meant. See inline on > patch > >> > >> diff --git a/zebra/rt_socket.c b/zebra/rt_socket.c > >>> index 4d0a7db..9012280 100644 > >>> --- a/zebra/rt_socket.c > >>> +++ b/zebra/rt_socket.c > >>> @@ -68,7 +68,7 @@ sin_masklen (struct in_addr mask) > >>> > >>> /* Interface between zebra message and rtm message. */ > >>> static int > >>> -kernel_rtm_ipv4 (int cmd, struct prefix *p, struct rib *rib, int > family) > >>> +kernel_rtm_ipv4 (int cmd, struct prefix *p, struct rib *rib) > >>> > >>> { > >>> struct sockaddr_in *mask = NULL; > >>> @@ -245,7 +245,7 @@ sin6_masklen (struct in6_addr mask) > >>> > >>> /* Interface between zebra message and rtm message. */ > >>> static int > >>> -kernel_rtm_ipv6 (int cmd, struct prefix *p, struct rib *rib, int > family) > >>> +kernel_rtm_ipv6 (int cmd, struct prefix *p, struct rib *rib) > >>> { > >>> struct sockaddr_in6 *mask; > >>> struct sockaddr_in6 sin_dest, sin_mask, sin_gate; > >>> @@ -363,33 +363,32 @@ kernel_rtm_ipv6 (int cmd, struct prefix *p, > struct > >>> rib *rib, int family) > >>> > >>> #endif > >>> > >>> +static int > >>> +kernel_rtm_ipv6 (int cmd, struct prefix *p, struct rib *rib) > >>> > >> > >> I assume this should be > >> kernel_rtm - not kernel_rtm_ipv6 > >> (otherwise you have a duplicate for kernel_rtm_ipv6() and a loop > >> in case of AF_INET6) > >> > >> > >> +{ > >>> + switch (PREFIX_FAMILY(p)) > >>> + { > >>> + case AF_INET: > >>> + return kernel_rtm_ipv4 (cmd, p, rib); > >>> + case AF_INET6: > >>> + return kernel_rtm_ipv6 (cmd, p, rib); > >>> + } > >>> + return 0; > >>> +} > >>> + > >>> int > >>> kernel_route_rib (struct prefix *p, struct rib *old, struct rib *new) > >>> { > >>> - struct rib *rib; > >>> - int route = 0, cmd; > >>> - > >>> - if (!old && new) > >>> - cmd = RTM_ADD; > >>> - else if (old && !new) > >>> - cmd = RTM_DELETE; > >>> - else > >>> - cmd = RTM_CHANGE; > >>> - > >>> - rib = new ? new : old; > >>> + int route = 0; > >>> > >>> if (zserv_privs.change(ZPRIVS_RAISE)) > >>> zlog (NULL, LOG_ERR, "Can't raise privileges"); > >>> > >>> - switch (PREFIX_FAMILY(p)) > >>> - { > >>> - case AF_INET: > >>> - route = kernel_rtm_ipv4 (cmd, p, rib, AF_INET); > >>> - break; > >>> - case AF_INET6: > >>> - route = kernel_rtm_ipv6 (cmd, p, rib, AF_INET6); > >>> - break; > >>> - } > >>> + if (old) > >>> + route |= kernel_rtm (RTM_DELETE, p, rib); > >>> > >> > >> Changed to > >> route |= kernel_rtm (RTM_DELETE, p, old); > >> > >> + > >>> + if (new) > >>> + route |= kernel_rtm (RTM_ADD, p, rib); > >>> > >> > >> and changed to > >> route |= kernel_rtm (RTM_ADD, p, new); > >> > >> (You removed the declaration of “rib” above - so rib is undefined) > >> > >> > >>> if (zserv_privs.change(ZPRIVS_LOWER)) > >>> zlog (NULL, LOG_ERR, "Can't lower privileges"); > >>> > >> > >> Attached is a updated patch > >> > >> With the changes it fixes the specific issue I’ve mentioned. I have not > >> verified the > >> patch on Linux yet. Will do a full run with this patch to see how many > of > >> my approx > >> 20..30 failing FreeBSD testcases it fixes (I assume many to all…) > >> > >> - Martin > >> > >> > >> _______________________________________________ > >> Quagga-dev mailing list > >> [email protected] > >> https://lists.quagga.net/mailman/listinfo/quagga-dev > >> >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
