search

pali / igmpproxy

IGMP multicast routing daemon
Other
149 stars 77 forks source link

MAX_IF 40 not sufficient for freebsd #62

Open Uglymotha opened 4 years ago

Uglymotha commented 4 years ago

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 commented 4 years ago

freebsd_max_if.zip

pali commented 4 years ago

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 commented 4 years ago

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

pali commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

pali commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Uglymotha commented 4 years ago

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

pali commented 4 years ago

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 commented 4 years ago

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