troglobit / mrouted

The original DVMRP (dynamic multicast routing) implementation for UNIX
https://troglobit.com/projects/mrouted/
Other
83 stars 17 forks source link

Have mrouted become member of some statically configured groups #58

Closed Bischoff closed 1 year ago

Bischoff commented 1 year ago

This PR addresses the issue #54 .

troglobit commented 1 year ago

Sorry for the late review, this is nice work! Except for a single omission, there's only minor things which I can fix up later. You need to add handling for SIGHUP/restart() to ensure mrouted can also leave groups it joins if the users removes them (or some of them) from the .conf file -- leave + free should also be done at shutdown/cleanup().

Bischoff commented 1 year ago

Sorry for the late review, this is nice work! Except for a single omission, there's only minor things which I can fix up later. You need to add handling for SIGHUP/restart() to ensure mrouted can also leave groups it joins if the users removes them (or some of them) from the .conf file -- leave + free should also be done at shutdown/cleanup().

Oh, right! I thought about it, and then forgot!

My SUSE hackweek (https://hackweek.opensuse.org/22/projects/mrouted-join-multicast-groups-via-ipip-or-gre-tunnels) is over, so I will have to do it during my free time. Hopefully, I won't make you wait too much.

Feel free to mention the "minor things" here, so I fix them as well.

Bischoff commented 1 year ago

While I had a first look at the cleanup/restart implementation, I noticed someting that I missed in the first place :disappointed: .

For each interface, the static groups are implemented through 2 lists:

The reason for doing so is that we have both static groups, and groups obtained from incoming group reports.

I am wondering if it would be a good idea to do the same, by concerns of symmetry, with static joins. In that case, I suggest we have four lists:

Of course it's not required, as we have only static joins for now.

I don't know. Maybe YAGNI and we should leave it like it currently is. What do you think? Am I overthinking it?

Bischoff commented 1 year ago

I pushed in a second commit the code for leaving the group on cleanup and restart.

It was tested with success with both Ctrl-C and mroutectl restart.

I do not like this implementation, because the other calls to k_leave() (for all routers group etc) are in stop_vif(), while this one is in stop_all_vifs(). But I am not sure we can do any better.

Criticism and suggestions welcome.

troglobit commented 1 year ago

Thanks, I'll have a look during the weekend. (Completely overloaded at $DAYJOB atm., sorry)

troglobit commented 1 year ago

Looks good to me, let's merge this! :partying_face: