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

Dynamic IfDesc Table #63

Closed
wants to merge 12 commits into from
Closed

Dynamic IfDesc Table #63

wants to merge 12 commits into from

Conversation

Uglymotha
Copy link
Contributor

Fixes issue #62 and build errors on freebsd because of incorrect placement of includes.

Rewrite of ifvc.c to switch from globally defined IfDesc array to dynamically allocated array.
Rewrite of buildIfVc and rebuildIfVc functions to use the new dynamicly allocated array and getifaddrs instead of socket io for enumerating interfaces.
Correctly implement rebuildIfVc to enable rescanvif option in config.
Fixed a typo and corrected placement of includes for freebsd.
Removed MAX_IF as it's no longer needed.
addvif/delvif should be called only for up/downstream interfaces in rebuildIfVc
To prevent builderror on GCC 9
@pali
Copy link
Owner

pali commented Apr 30, 2020

Ok, so you have already figured out that there is automatic compile check for every pull request and you have already fixed code as to be compatible with more compiler versions...

Some more comments:

  • If diff there are lot of whitespace changes, which make only noise and make reviewing patches more complicated. If there are no reasons for whitespace changes, please remove them.
  • Squash "fixup" commits into some relevant previous commit (see git rebase --interactive) and then do force update (see git push --force); every commit in history should be compile-able
  • If it is not really necessary, do not try to change indentation of code as it just increase diff

@Uglymotha
Copy link
Contributor Author

Over the last few days my rework of the code has become increasingly extensive. Igmpproxy was indeed a good project to get back into programming, it's definitely been fun.
In some places the code was just ancient, like using socket io to get interface info instead of getifaddrs(). In other places code was shabby, in some places bad and in a couple absolutely terrible, like the main loop and queue timing. The project seems to hav esuffered from repeated unfinished attempts to implement functionality. Leading to a bit of a mess. Outside of the actual igmp functionality it's all redone the way it should be. Lots a functions were altered to more readable and flexible code, some functionality split of into new functions and some removed.

I currently have the interface and configuration management working correctly as far as I am able to test, SIGHUP is functional and main loop timing is now implemented in just 12 simple lines of code, using 1 call to clock_gettime. instead of the some 60 lines it was before. This also fixes issue #58 without adding another shabby patch to a completely incorrectly implemented timing.

I am now reworking the callout queue, when that's finished I'll do some more testing of my own before I am confident in releasing the new code. Hopefully I'll be able to get a version of this all in my own repo today.

Leaves the question of how to proceed. Opening a new pull request for all the changes when I'm finished or maybe fork off completely. My preference would be the first.

@pali
Copy link
Owner

pali commented May 5, 2020

Please separate changes (ideally to more pull reaquests) as reviewing one big change is hard. Also please update this pull request as I mentioned in previous comment so I can look at it.

@Uglymotha
Copy link
Contributor Author

After some more testing, I am now confident to release the new code base.

Closing this PR and opening a new one, is it has become much more than just dynamic ifdesc tables.

@Uglymotha Uglymotha closed this May 8, 2020
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.

2 participants