On Thu, Jul 2, 2015 at 1:35 AM, Timo Teras <[email protected]> wrote:
> 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. > I've been thinking about this some and I'm coming to the opinion that all daemons should be enabled by default. Disabling daemons by default will hide some compile failures that could be introduced by changing common dependent code. And coincidentally I've seen a couple bugs be fixed in the past couple of months through people having different compile flags from configure. It seems like it would be better to enable and let things shake out where they may. What daemons that are included in a package is a packaging issue that can be handled by a --disable-<daemon> as part of that packages compilation. I say leave it the way you have it. > > 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. > I've not experienced this. I don't believe we touch this code and I get usable core dumps all the time. I'm particularly good at writing code that core dumps :) Perhaps something strange is going on? I just went and looked at your code again as that I wanted to compare to our code. You've already removed the changes to lib/sigevent.c > > > 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. > Looks like you've thought about this a bit more than I have. I would prefer moving of the nhrpd/list.h into lib/list.h, and having the start of submittal of code that consolidates on one link list implementation as you have suggested. Perhaps adding some verbiage to the other linked list functions about moving over? donald > > 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
