Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to timing and igmpproxy #72

Closed
wants to merge 1 commit into from
Closed

Conversation

Uglymotha
Copy link
Contributor

This commit contains a number of changes to the timing structure and
main event loops.
Also several variables and structs were moved to igmpproxy.h in preperation
to coming changes.
Includes minor changes to config.c and request.c to include new blacklists
for interfaces.
callout.c is modified to a simpler and more stable algorithm.
igmpproxy.h and igmpproxy.c are sanitized and adopted to the new callout queue.

FIXES: #58

@Uglymotha Uglymotha force-pushed the timing branch 2 times, most recently from 71c563e to 9517ac5 Compare May 30, 2020 12:48
@pali
Copy link
Owner

pali commented Jun 4, 2020

Please, please, do not mix different things into one pull request and one commit. Fixes for timings should not contain coding style changes or "several variables and structs were moved to igmpproxy.h". Also "preparation for other changes" should be also in separate pull request.

Simpler and smaller is better!

@Uglymotha
Copy link
Contributor Author

Uglymotha commented Jun 20, 2020

It took a while but I have update the PR. I removed much of the unrelated changes. Though some still remain. The timing algorithm was also updated to be much, much more accurate than before:

0:01:12:5243 About to call timeout 1 (#1) - Age Active Routes - Missed by 282us
10:01:33:5251 About to call timeout 2 (#1) - General Query - Missed by 1043us
10:01:43:5255 About to call timeout 3 (#1) - Age Active Routes - Missed by 258us
10:02:04:5264 About to call timeout 4 (#1) - General Query - Missed by 1114us
10:02:14:5275 About to call timeout 5 (#1) - Age Active Routes - Missed by 1082us
10:04:09:5277 About to call timeout 6 (#1) - General Query - Missed by 1226us
10:04:19:5289 About to call timeout 7 (#1) - Age Active Routes - Missed by 1147us
10:06:14:5288 About to call timeout 8 (#1) - General Query - Missed by 1013us
10:06:24:5292 About to call timeout 9 (#1) - Age Active Routes - Missed by 193us
10:08:19:5299 About to call timeout 10 (#1) - General Query - Missed by 916us
10:08:29:5311 About to call timeout 11 (#1) - Age Active Routes - Missed by 1116us
10:10:24:5310 About to call timeout 12 (#1) - General Query - Missed by 919us
10:10:34:5322 About to call timeout 13 (#1) - Age Active Routes - Missed by 1122us
10:12:29:5320 About to call timeout 14 (#1) - General Query - Missed by 922us
10:12:39:5322 About to call timeout 15 (#1) - Age Active Routes - Missed by 78us

As you can see, I also included a minor change to syslog.c, is now timestamping logs to stderr. Also switched to unsigned long for timer id instead of int. just a little but of future proofing. Now if somebody were to be able to fire off 1 billion timers per second it would still take about 600years to roll over. :)

@Uglymotha
Copy link
Contributor Author

Over the past 2 weeks I have been adding a bunch of functionality, testing a lot and weeding out bugs. I am now confident the code of my rewritten version is solid. A lot of work, effort, blood, sweat and tears went into it. I do think it has come together real nice.

My version now features:

  • Major improvements to configuration parsing. It is basically fool proof now, no more error exits on config parsing. Important for config reload functionality. Lists can now be confiogured like altnet 1.2.3.0/24 1.2.4.0/24

  • 3 process signals: SIGUSR1 for a config reload, SIGUSR2 for interface rebuild and SIGHUP for both. (Also undocumented SIGURG, for cleanup and restart, I use this as a simple memory leak detection)

  • Timed reload of configuration and timed rebuild on interfaces.

  • Most parameters are fully configurable now (like query interval / response).

  • Added defaultratelimit and defaultthreshold for interfaces.

  • Added optional logging to file. Loglevel is set in conf, und -v cl options removed. Logging to stderr and file is properly timestamped.

  • Full and correct support for multiple upstream interfaces.

  • igmpproxy now participates in IGMP Querier Election. Can be switch off.

  • Full per interface black and whitelists.

  • Switched from recvfrom() to recvmsg() with control message, so the incoming interface for IGMP requests is determined 100% accurately. Instead of being deduced from src and dst.

  • Route activation is now checked against src bwl and dst group bwl for upstream interfaces only.

  • Fixed a ton of issues I found during my testing and working out of scenario's. Like (definitely not complete list):

    • Correct handling of IP aliases on FreeBSD.
    • Correct handling of bridge interfaces. On FreeBSD (I suspect Linux will behave the same), since a raw socket is used, every IGMP request is received twice, 1 for the parent and 1 for the child interface. This was causing issues with aging since igmpproxy did not track what groups were being queried from what interface.
    • Routes were set to last member mode, but never reset to normal. This could cause issues with Leave messages being followed by join messages for the same group, since the group was unjoined, but not set to uinjoined and hence never rejoined. Most likely cause of issue Stream will stop if you stop and start the stream very fast. #47. Fixed by keeping track of previous state and reset to normal in internAgeRoute().
    • setRouteLastMemberMode() was resetting route vifBits. This could lead to interrupted stream, when a report was received on a different interface, just after the leave. The route was then updated by insertRoute() with the vifBits of the original interface cleared. the function now uses a local copy of the vifBits to do it's logic and only clears ageVifBits.
    • struct RouteTable was defined as a doubly linked list, but always used as single linked list, so I made it a single linked list, and removed lots of unnecessary complexity.
    • The code contained many frivolous and unnecessary code. I have removed all references to and bad code by somebody named aimwang. For instance rttable.c now contains less lines of code then the original, though it is handling the most complex part of the new version. Updating of routes due to changes in config / interfaces states.
  • Group Specific queries are now tracked by interface / group and aging status.

  • Full handling and reevaluation of BWL after config reload. IE if you change a group to be blacklisted, it will be removed / unjoined etc.

  • Correct handling of changes to hashtablesize, routes are realloced with the appriopriate amount of memory if hashtable size is changed. Downstream host tracking will have to be reinitialized if this happens of course.

  • Written in one unified style. All pointer and list logic is now handled the same. Like list lookups done inside for loops definitions themselves. Always if (prt / ! ptr) instead of mixing it with ptr == null.

I think I basically turned it into a 1.0 version. I have been testing with both Linux and FreeBSD, in a chained igmpproxy set up. Had to set this up, because Linux uses in_pktinfo for recvmsg and BSD sockaddr_dl.

This commit contains a number of changes to the timing structure and
main event loops.
Includes minor changes to config.c and request.c to include add name for timers.
callout.c is modified to a simpler and more stable algorithm.
igmpproxy.h and igmpproxy.c are sanitized and adopted to the new callout queue.

FIXES: pali#58

Fixed a few typos and comments

Updated Timing Algorithm

Timers are now kept with a struct timespec, so they are much mnore acurately scheduled.
Added a name to the timeoutqueue struct, improves debugging timers by a bunch.
Renamed timer functions to be more in line with style
time_ageQueue() now returns ns until next timer if scheduled in less then 1s.
                this makes timer execuation really accurate. Timers are not missed by more than a few 1/100s
Updated syslog.c to add timestamp when logging to stderr.

void debugQeueu(VOID)
@pali
Copy link
Owner

pali commented Jan 9, 2021

Hello! Could you split changes into smaller pieces? One commit = one logical change? As I said it is really hard to review & merge big pull request which mix lot of features into one commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

igmpproxy timer execute error
2 participants