MAX_IF 40 not sufficient for freebsd

[Uglymotha]

This has been reported before I think, but because freebsd creates multiple logical interfaces for one actual, the default value of 40 for MAX_IF is often not sufficient. In my case even 200 was not enough, since my system has 16 phys ints, several laggs, 10 vlans per lagg and bridges across these.

I created the attached patch to fix this. I can create a pull request if you are willing to accept the patch.

[Uglymotha]

freebsd_max_if.zip

[pali]

I know about this issue, but your patch does not fix it. It just increase max limit to some random number. Real fix for this problem is to remove upper limit and change static memory allocation to dynamic one based on number of interfaces in system.

[Uglymotha]

Shouldn't that be fairly simple to acheive by using getifaddrs() / freeifaddrs() ?
These functions already use dynamic memory allocation.

[pali]

Well, I have not looked at it deeply how to implement it. If you this it is simple, feel free to open a pull request.

[Uglymotha]

First off, it's been some 20-25 years since I've done any real programming in c. Been looking through and analysing the source code yesterday and igmpproxy seems a basic enough project to get back into it. Like many people I currently have some time free to do this. So here's what I now think about solution to this issue.

1. Move the global definition of the IfDescVC[] array and *fDescEp into main(). It's good programming practice anyway.
2. Initialize the array at start-up based on the actual nr of interfaces in the system. The amount of memory needed will be known, also static (have to analyse the RebuildIfVc function yet to see how dynamic the array might be there)
3. Pass the pointers as arguments to the functions that need them.

Functions in ifvc.c are not used that much and I think I already have much of the above covered.

[Uglymotha]

Oh and maybe have MAX_IF be configurable. igmpproxy is often run on memory restrained systems

[pali]

Oh and maybe have MAX_IF be configurable. igmpproxy is often run on memory restrained systems

struct IfDesc is 50-60 bytes long. You would have to about 100 000 interfaces to hit memory limit problems (10 MB) on small memory-limited systems. But I guess your memory-limited system would not be able to process so much interfaces as cost for it would be larger then cost for for igmpproxy.

So I think it does not make sense to complicate it. If your system has enough memory for having interface configured in system, there would be also additional 60 bytes for this interface usable for igmpproxy.

[Uglymotha]

Oh the sweet memories. It never works out the way you think when you finally figure out how stuff actually works. I currently have a working igmpproxy with dynamically allocated IfDesc table. In the end I did:

1. Convert the global definition of the IfDescVc array to a struct of a begin and endpointer and the nr of interfaces for reference.

2. Complete rewrite of the buildIfVc and rebuildIfVc functions. The latter was especially lacking the way it was. Instead of using sockety stuff for enumerating interfaces I switched to getifaddrs(), this is an atomic function, so will eliminate any race conditions when interfaces arrive, or dissapear during the enumeration. rebuildIfVc I think was functionality that was not completely implemented, it now is. It was not accounting for freeing the malloced subnetlist in the IfDescVc, which may lead to memory leakage. It was not accounting for new interfaces that were not present during start up (configureVifs was not run. so subnestlist pointers not initialized). And not joining mcrouter groups for new interfaces. Code is much cleaner and understandable now and mallocs correctly freed.

3. The rescanvif option can be used in config. rebuildIfVc now correctly goes through the process of creating a new IfDesc table, comparing it with the old. Calling defVIF for any removed interfaces. And calling AddVIF and joining mcrouter groups for new downstream interfaces. Documentation updated accordingly.
   igmpproxy-interfaces-patch.zip

4. Fixed a few typos here and there.

5. Fixed a build issue on freebsd due to incorrect placement of includes in igmpproxy.h.

Attaching a patch for review.
igmpproxy-interfaces-patch.zip

[pali]

Could you please post patches in normal way, via git repository and pull request, and not in zip files which github cannot process?

[Uglymotha]

Please ignore last patch, attached incorrect version.
igmpproxy-interfaces.zip

[pali]

Well, could you please really post patches in preferred way via git pull request? This is really hard for me review patches in this way. And also now if you have posted 3 files and so... Patches in pull request can be easily updated to "correct version".

[Uglymotha]

Looking into creating a pull request. In my good ol' days of programming there was only svn. :)