Code Rewrite

[Uglymotha]

Rewrite of the base code to fix issues #62 #32 #64

Overview of changes:

• Rewrite of ifvc.c to account for more dynamic handling of interfaces in system. buildIfVc() was changed to use getifaddrs() instead of socketio to retreive interface info. rebuildIfVc() now uses buildIfVc() instead of doing 90% the same thing.
• Rewrite of callout.c and main event loop to allow for better and simpler timing algorithm. The main loop now no longer needs to care about when timers need to be executed, but only to run the queue aging once every timer resolution (1s).
• Rewrite of config.c to allow for more dynamic handling of interfaces and configuration. for this mroute-api addVif() function was modified for updating or building new vifs when interfaces or configuration change, Split of creation of vifs from igmpproxyInit() into createVifs() function. Added reloadConf() function for reloading configuration on SIGHUP and periodically.
• Added several configuration options (rescanconf / loglevel). Documentation updated. -v command line options are not yet removed to provide backwards compatibility. Fixed rescanvif to do what it actually needs to do.
• Implemented SIGHUP handler to reload configuration / rebuild interfaces.
• Fix for compilation errors on FreeBSD due to incorrect placement of includes on this platform.
• Minor updates to mrouteapi.c (AddVif / Delvif) and rttable.c (clearroutes) to allow for dynamic interfaces.

All this will make the code base much more flexible and readable. The above was achieved while in total 60 lines less code are required. The main event loop timing and timer functions were probably some of the worst I have ever come across. The following snippet of debug logging I did with the old timing says it all. Rebuild timer jumps from 60 secs to 38, within one sec after timers were added. This was because all sorts of weird calculations were being made instead of just maintaining an execution queue in the right order. In turn this was causing timers to execute out of order and/or early.
rebuildtimer: 22 60
About to call timeout 20 (#0)
SENT Membership query from 192.168.1.254 to 224.0.0.1
Sent membership query from 192.168.1.254 to 224.0.0.1. Delay: 10
Created timeout 23 (#0) - delay 10 secs
(Id:23, Time:10)
(Id:22, Time:28)
Created timeout 24 (#2) - delay 87 secs
(Id:23, Time:10)
(Id:22, Time:28)
(Id:24, Time:87)
rebuildtimer: 22 38

TODO:
Currently only changing of downstream interfaces is supported and this relies on normal aging of routes (stream will continue until route ages). This is because the actual relinking of routetable to changed interfaces is not yet implemented. This will be my next step in the rewrite.

[pali]

Hello! This is huuuuge change. I see there changes to different features / parts and it would be hard for me and take lot of time to read and review whole change as is. I do not have such many time for igmpproxy project, so please split changes into smaller independent pieces, put them into separare pull request, so they can be reviewed and merged independently. It would really helps me to process all your changes more effectively (e.g. there are some documentation changes which I can merge immediately, but due to how github works, only whole pull request can be merged). One big pull request with one big git commit is really hard to proces.

[Uglymotha]

I understand that it's huge, but like said it is a rewrite of the code base. It will be very difficult if not impossible to split it over multiple PRs, because it all ties together. It is as in Franks Herbert's words: Rot in the core spreads outward. I started out by investigating how I could fix my own issue with interfaces on freebsd and in doing so I discovered that the whole procedure of building interface had to be rewritten were it to be fixed the right way.
To do that I had to go and redo the way interfaces were linked to vifconfigs and actual mc vifs. In doing so I saw that the whole timing of the main event loop and queue were flawed and I had to redo that, because to support dynamic interfaces the function had to be timed instead of indiscriminately running every event loop iteration.
So since I have time on hands I decided that the best thing to do was a rewrite of the core process flow to make it more flexible and better readable. More so since much of what I saw in the code during my review was hard to read and follow. Unfortunately if you get to the core you will have to change all the functionality accordingly from the inside out, And I'm not finished yet, I am still to do the request and rttable, to allow for dynamic upstream interfaces.
This really is the first iteration of the rewrite I am confident to release. I may be able to split of some of the new functionality, like SIGHUP handling into separate PRs. But these are just a few lines of code really, thanks to the better core process flow. SIGHUP for instance is just a call to reloadConfig(), which in turn calls rebuildIfVc() which itself uses buildIfVc(). The largest changes, those in config.c, ifvc.c and callout.c (which has more than 50% reduced code with much better accuracy, simpler use and an understandable algorithm) tie together atomically. I have tested on freebsd and Linux for as much as I can. Though on Linux I cannot test actual MC traffic, since I have not touched request.c and rttable.c (outside of minor change to clearRoutes()) I do not expect issues there.

What may be an option is to have this rewrite split off into it's own branch, I would definitely like to see people try the new rewrite I did. Something will surely come up. :)

To give you an idea of the simplification involved. Go look at current version of callout.c. The new queuing algorithm is just this:
if (!queue) queue = node;
else {
// chase the queue looking for the right place.
for ( i++; ptr->next && node->time >= ptr->next->time; ptr = ptr->next ) i++;
if ( ptr == queue && node->time < ptr->time ) {
// Start of queue, insert.
queue = node; node->next = ptr;
} else if ( ptr->next ) {
// Middle of queue, insert
node->next = ptr->next; ptr->next = node;
} else {
// End of queue, append.
ptr->next = node;
}
}

Place a new timer in the right position according to order of execution and aging the queue boggles down to just this:
while (ptr && ((ptr->time <= curtime.tv_sec) || ( curtime.tv_nsec >= 500000000 && ptr->time <= curtime.tv_sec-1))) {
my_log(LOG_DEBUG, 0, "About to call timeout %d (#%d)", ptr->id, i);
struct timeOutQueue *tmp = ptr;
if (ptr->func) ptr->func(ptr->data);
queue = ptr = ptr->next;
free(tmp);

Execute any expired (or about to be expired by .5s) timer and the main event loop now has to eveluate just this:
clock_gettime(CLOCK_MONOTONIC, &curtime);
difftime.tv_sec = curtime.tv_sec - lasttime.tv_sec;
if (curtime.tv_nsec >= lasttime.tv_nsec ) {
timeout->tv_nsec = 999999999 - (curtime.tv_nsec - lasttime.tv_nsec);
} else {
timeout->tv_nsec = 999999999 - (1000000000 - lasttime.tv_nsec + curtime.tv_nsec); difftime.tv_sec--;
}
if ( difftime.tv_sec > 0 || timeout->tv_nsec < 10000000 ) {
timeout->tv_nsec = 999999999; timeout->tv_sec = 0;
lasttime = curtime;
age_callout_queue(curtime);
}

Just set the timeout to 1s - the time spent since last age. And if it is past aging time or near aging by .01s we age and reset timeout.

This few simple steps took over 200 lines of code using flawed logic and mathematics before my rewrite. The new algorithm is simple and fairly accurate (for a single threaded process), only on very slow or busy systems the timer execution may overshoot by a second. I see no need in complicating it more than this to run aging twice per timer resolution.

[pali]

Really try to separate changes to more commits (or pull requests) to have it smaller. You wrote that SIGHUP handler code would just few lines, and that is great! Also I cannot see how documentation change needs to be bound together with other changes. Really try to deliver code in smaller changes. Otherwise it took me too much time to process all these changes.

[Uglymotha]

I have thought about this a little and I think I can get away with first introducing the changes in the main event loop timing and timer queue. And maybe introduce a few new short still unused functions like reloadconfig / rebuildifvc. For this the call to rebuildIfVc() in the main loop will need to be removed, but it is not doing anything useful at the moment anyways.

Next I can create a PR for introducing the changed dynamic interfaces code, this will be fairly large that's unavoidable.

Finally I can do the new configuration options and documentation.

[pali]

I would be happy with anything which split one big change into more smaller which could be reviewed / commented / read and merged separately.

[Uglymotha]

Closing this PR and opening new one for changes to timing.