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

increase MAX_IF to 100 #46

Closed
wants to merge 1 commit into from
Closed

increase MAX_IF to 100 #46

wants to merge 1 commit into from

Conversation

fichtner
Copy link

Several users of OPNsense and pfSense report a persistent error
with their configuration: "There must be at least 1 Vif as upstream."

It seems that there is no real error in igmpproxy, but rather with
the way that network addresses are looked up prior to starting up.
This can break when many addresses are present in the system.

There is no exact science for the increase to 100 and previous custom
patches on both projects have had this raised to 80 back when version
0.1 was used. There could be further work required to dynamically
parse addresses in a way that this raised limit does not cause further
issues in the future.

See also: opnsense/plugins#590
See also: https://redmine.pfsense.org/issues/8935

Several users of OPNsense and pfSense report a persistent error
with their configuration: "There must be at least 1 Vif as upstream."

It seems that there is no real error in igmpproxy, but rather with
the way that network addresses are looked up prior to starting up.
This can break when many addresses are present in the system.

There is no exact science for the increase to 100 and previous custom
patches on both projects have had this raised to 80 back when version
0.1 was used.  There could be further work required to dynamically
parse addresses in a way that this raised limit does not cause further
issues in the future.

See also: opnsense/plugins#590
See also: https://redmine.pfsense.org/issues/8935
@fichtner
Copy link
Author

As pointed out via #38 (comment) and further patch analysis this seems to solve the issues at hand, see bug references in OPNsense and pfSense.

I want to upstream more patches from FreeBSD so 0.2.2 has no priority. FreeBSD port was updated in parallel: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231800

@pali
Copy link
Owner

pali commented Sep 29, 2018

I really do not like an idea to change upper limit from one magic constant to another without exact reason. You also wrote There is no exact science for the increase to 100.

Therefore rather completely remove that array limit and allocate array dynamically to size which is needed on running system.

And if you really really think that there should be some upper limit then make it configurable via config file.

@fichtner
Copy link
Author

I agree, your project, your rules. I also think this is the best way forward for the time being for approaching a bug that existed for many years. And it’s hard to argue against it from a technical perspective: it’s in line with your code, it doesn’t introduce regressions and it doesn’t require additional vetting.

@pali
Copy link
Owner

pali commented Sep 30, 2018

Main problem is that I still do not see "why 100". Why it is enough now and also for future? It does not matter if it is one line change or 100 lines change in code. Basically it is not a fix, just workaround about which nobody knows why is working and if it would work also in future.

@fichtner
Copy link
Author

Yes, and I said so initially. We should be beyond this talking point by now, but we are not.

@fichtner fichtner closed this Jan 3, 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