On Wed, 1 Jul 2015 20:42:39 -0400 Donald Sharp <[email protected]> wrote:
> nhrp is turned on by default. Typically this is the reverse for new > daemons from what I can tell( Feel free to correct me if I am wrong > here ). Babeld was enabled by default. Pimd seems disabled by default. Isisd was long time disabled by default, as it was known to have issues. So it seems to have been "disabled by default if it's expiremental or unstable". But I could wrong too. I'm fine to change it to disabled by default if so wished. I also need to fix it so that it cannot be enabled without netlink. > lib/sigevent.c - Any particular reason this code was changed? > Especially since you #if 0'ed it, if this was intentional, please > just remove the code. I suspect this was a testing change? Yeah, that was for my testing only. I should probably make that a configure switch, or run time option. Doing anything in the signal handler makes the core dumps useless. > nhrpd/list.h - This really belongs in lib, and in fact quagga already > has link list functionality. Did I catch the tail end of this > discussion on the quagga irc channel a few weeks back between you and > Paul? What was the conclusion there? From a very quick glance the > two implementations look orthogonal enough that they could be both > put in lib and then work could be done to move to the > future( whatever the future is ). Yes. We had a discussion of this on IRC. There was no conclusion, but we agreed to go through the discussion in the mailing list. So this is a good place to start that discussion :) The sad fact is that there is several trivial linked-list instances in quagga code currently: lib/queue.h and lib/linklist.[ch] are the existing APIs. But various lib/ and zebra/ structures implement it inline too (just grep for 'next'). And now I'm adding the linux-kernel style link lists. I think some argued that it'd be better using one of the existing ones. But I argue that the linux-kernel style api is simpler, simpler to use, and faster. To make code uniform it'll require refactoring most of the existing code so I hope we could pick the best API. lib/queue.h is the well known BSD API. It performs well, but the usage side is not so simple as to define the nodes you need to instantiate macroes. You also need to pass the parent structure pointer + member name to all list modifiers, making the api somewhat complex. lib/linklist.h is quagga specific where the list nodes are allocated separately. Doing allocs increases code size, failure paths and slows down code (cache contention, extra code ran). Though, this does have a place if one list node needs to be present in multiple lists. If such uses exist, lib/linklist.h should be rewritten in terms of the simpler list api. And the inlined implementations should just go away. They are error prone, and make the code harder to understand and maintain. If nhrp/list.h goes to lib, I'm volunteering to remove lib/queue.h and replace its usage with list.h. So we will not go to any worse state. I can also see if I can fix some of the other places too. > nhrpd/netlink.[ch] - Do you forsee a future where this netlink code > will be used elsewhere? If so then shouldn't this interface be in > zebra? BTW, not just for this code, but this comment also goes for > the setsockopt interface in pimd for multicast routes. It could be used to simplify the existing zebra netlink code. However, it relies on the zbuf code which is more or less replacement for stream.[ch]. It'd probably need similar discussion as for the linked lists. So I'd probably at this point just keep it as-is in the nhrpd/ dir until list.h is handled first. > In all honesty, it's 7k lines of code. It's too big to review, this > sure does look like a step in the right direction though, so it gets > my ACK :) Thanks. _______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
