Changes to timing

[Uglymotha]

So here's the first new pull request with changes to the timing structure. This fixes issue #59 and build errors on freebsd due to incorrect placement of includes in igmpproxy.c and igmpproxy.h

To make this compilable and functional I also have to introduce a couple of (as of yet) unused functions. These are reloadConfig() and createVifs(), a function that will be used to dynamically update and create vifs during config reload. JoinMcRouterGroup(), a split of funcionality from initRouteTable(), also to be used for dynamically managed interfaces. The function clearAllRoutes was changed to clearRoutes, it will also be used in dynamice interfaces to remove routes for a certain upstream if, hence the name change.

Some new structs are also defined and some change because they are used as such in the new functions.

[pali]

When writing a new code, try to use consistent style. E.g. put if (...) and its following function() call into two lines (not as oneliner). Same for for (...) if (...) break; put into 3 lines, not one long oneliner. Also in new code try to use consistent spacing for constructions if (var), for(var... etc.

[Uglymotha]

Can’t find this comment, did you already delete it? It’s of course 500.000.000ns ie 0.5s.

From: pali notifications@github.com Sent: Friday, May 8, 2020 10:24 PM To: pali/igmpproxy igmpproxy@noreply.github.com Cc: Sietse van Zanen sietse@wizdom.nu; Author author@noreply.github.com Subject: Re: [pali/igmpproxy] Changes to timing (#66)

@pali commented on this pull request.

---

In src/callout.chttps://github.com/pali/igmpproxy/pull/66#discussion_r422355021:

• }

-

• queue = ptr;

• if (last) {

• last->next = NULL;

• }

• / process existing events /

• for (ptr = _queue; ptr; ptr = _queue, i++) {

• _queue = _queue->next;

+void age_callout_queue(struct timespec curtime) {

• struct timeOutQueue *ptr = queue;

• int i = 1;

• if (curtime.tv_sec == 0) clock_gettime (CLOCK_MONOTONIC, &curtime);

• while (ptr && ((ptr->time <= curtime.tv_sec) || ( curtime.tv_nsec >= 500000000 && ptr->time <= curtime.tv_sec-1))) {

What does 500000000s means?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/pali/igmpproxy/pull/66#pullrequestreview-408502765, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC3S6AIQJE246ORMZND47U3RQRS5BANCNFSM4M4I5T4Q.

[Uglymotha]

When writing a new code, try to use consistent style. E.g. put if (...) and its following function() call into two lines (not as oneliner). Same for for (...) if (...) break; put into 3 lines, not one long oneliner. Also in new code try to use consistent spacing for constructions if (var), for(var... etc.

I have adapted to the style used in the file, so it may be little bit different, because styles were different across files. For oneliners, I really like to use them when the actions involved are short and tied together. Like inserting into a list. I learned programming on the ZX Spectrum and C64. On these memory restrained systems you learn that having code on separate lines adds 2 bytes per line to your code size. :)

Also a logics table with few short actions, like this from createvifs: switch (oDp->state) { case IF_STATE_DISABLED: switch (Dp->state) { case IF_STATE_DISABLED: { continue; } case IF_STATE_DOWNSTREAM: { oDp=NULL; join=1; break; } case IF_STATE_UPSTREAM: { oDp=NULL; break; } } break;

will be much harder to read if the statements are put on their own line.

[pali]

Can’t find this comment, did you already delete it? It’s of course 500.000.000ns ie 0.5s.

I already explained on github that it is mistake, it is 0.5s and then marked comment as obsolate. Apparently this explanation was not sent by email.

[pali]

I have adapted to the style used in the file, so it may be little bit different, because styles were different across files.

So please choose one style which is already used in igmpproxy and do not invent a new one. Because the result would be like in XKCD 927.

[pali]

About style, look at these your lines:

if ( ptr == queue && node->time < ptr->time ) {

while (ptr && ptr->id != timer_id)

if ( ptr ){

if(Dp->state == IF_STATE_UPSTREAM) {

if (upsvifcount >= MAX_UPS_VIFS) {

Every one has different style where are put spaces. Please when writing a new code, make this consistent.

[pali]

Also a logics table with few short actions, like this from createvifs:

That switch in createVifs is basically OK, optically it can be easier to read.

But such oneliners for/if for (struct IfDesc *Dp = TmpIfDescP.S; Dp < TmpIfDescP.E; Dp++) free(Dp->allowednets); if (ptr->func) ptr->func(ptr->data); are not used in current igmpproxy code style. So they should be on two lines.

Generally it is not a good idea to bring own/new coding style to existing project to which are contributing more people. Doing it just result in having code very hard readable as every part of file would use different coding style (what people bring). Also reformating existing code in such project like igmpproxy is not a good idea, again if it would be done every contributor to his favourite coding style, then there would be only formatting changes.

[Uglymotha]

I think I have processed your comments and updated style of my code to be consistent. I also set up travis-ci on my own repo, so next pr's will be without warnings and cleaner (hopefully).

I still left in reloadconfig() function. It's not complex function and will make the next pr a little shorter. I think it will be a good trade-off between size and nr of prs needed to get in all my changes. I you really want to I'll remove it.

Waiting for travis to confirm build, after that's done I'm going to squash the commits.

I may also sync al the commits to my master branch and then open up a new clean pr when we agree on all the changes.

[pali]

Hello! I see that you have added to this pull request new features like rescanconf or loglevel keys for config file. It is really intended for this PR about timings?

[Uglymotha]

oh crap, I accidently did a PR merge on my own repo in the wrong direction. Sorry, I'm still learning to work with git.

Let's see if I can fix this. May be necessary to open yet another one.

[pali]

You do not need to open a new pull requst. You can update any one. Just look for git reset --hard command and git push --force command. Second one will allow you to push any changes to github.

[Uglymotha]

Awesome, the incorrect merge has ben reverted. PR is back in order again. I think we should be close on agreeing the changes.

I will add some lines of code the the createVifs() function as in the meantime I have worked out the last part of dynamic interface handling. If downstream if transitions to disabled or upstream the routes are now updated to clear the vif bit for that interface, zero downstream hosts, set to last member state and start the gsq process. Part of this is done in createvifs, so adding that to the code for this PR.

[Uglymotha]

Over the pas week or so I have finished my rewrite. I also added black and whitelist functionality and the logic to reevaluate these on config reload. I also took the time to make everything unified in style and removed a lot of unused code. If you're interested take a lok at the rewrite branch in my personal repo.

The good thing is that we can now start changing everything from the inside out. So for cleanliness I will be opening another new smaller PR.