search

troglobit / pimd

PIM-SM/SSM multicast routing for UNIX and Linux
http://troglobit.com/projects/pimd/
BSD 3-Clause "New" or "Revised" License
194 stars 86 forks source link

Fix RP hold time computation #106

Closed jp-t closed 7 years ago

jp-t commented 7 years ago

This one has side effects, see commit message.

Not sure if I should take some extra caution steps for compatibility.

By the way, there is this fishy code in pimd.h:

/* TODO: 60 is the original value. Temporarily set to 30 for debugging.
#define PIM_BOOTSTRAP_PERIOD             60 */
#define PIM_BOOTSTRAP_PERIOD             30

The correct value is 60 per RFC, but changing it could render pimd less reactive maybe?

jp-t commented 7 years ago

There is another important side effect:

This comment in rp.c around line 380:

        /* TODO: We shoudn't have old existing entry, because with the
         * current implementation all of them will be deleted
         * (different fragment_tag). Debug and check and eventually
         * delete.
         */

is not true anymore. I added a debug log in that case, and I can see it very often. But I cannot figure out what is implied. Since the original code didn't expect this, it might be incomplete or buggy.

troglobit commented 7 years ago

Great work, as always!

I think the best way is to make it configurable, and default to the RFC value (60).

Wrt to your second comment, I'd wager that code (as well as the comment) is both incomplete and buggy. The original code was written for RFC2362, or rather was the basis for that RFC. Work on pimd continued after the publication of RFC2362, but was stopped by the original authors before work on RFC4601 was completed.

troglobit commented 7 years ago

I've restored the correct `PIM_BOOTSTRAP_PERIOD' to 60 sec in 9c69710.

Please feel free to file a separate issue for the issue in rp.c, around line 380, you mentioned above.