Re: [nf-failover] [conntrack-utils PATCH r7254 01/14] merge mod_alarm() into add_alarm()

Re: [nf-failover] [conntrack-utils PATCH r7254 01/14] merge mod_alarm() into add_alarm()

[Pablo Neira Ayuso]

Max Kellermann wrote:
>  include/alarm.h   |    4 +---
>  src/alarm.c       |   13 +++++--------
>  src/cache_timer.c |    5 ++---
>  src/sync-alarm.c  |   17 +++++++----------
>  src/sync-ftfw.c   |    8 +++-----
>  5 files changed, 18 insertions(+), 29 deletions(-)

Applied a similar version. Thanks.

BTW, I'm cc'ing to both netfilter-devel and netfilter-failover, next
time just include netfilter-devel since we're discussing development stuff.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [nf-failover] [conntrack-utils PATCH r7254 02/14] remove duplicate init_alarm() invocations

[Pablo Neira Ayuso]

Max Kellermann wrote:
> diff --git a/src/sync-alarm.c b/src/sync-alarm.c
> index f605586..062c0b4 100644
> --- a/src/sync-alarm.c
> +++ b/src/sync-alarm.c
> @@ -49,7 +49,6 @@ static void cache_alarm_add(struct us_conntrack *u, void *data)
>  {
>  	struct alarm_list *alarm = data;
>  
> -	init_alarm(alarm, u, refresher);

I guess that you wanted to remove init_alarm() from refresher() instead.
Fixed and applied. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [nf-failover] [conntrack-utils PATCH r7254 04/14] fix misoptimization for next_alarm calculation

[Pablo Neira Ayuso]

Max Kellermann wrote:
> do_alarm_run() determines the next_alarm time.  there is however a
> condition where this result comes wrong, because of "unsafe" side
> effects of list_for_each_entry_safe().  Example: imagine there are two
> alarms: A is due now, and B is due in 5 minutes.
> 
> When do_alarm_run() runs alarm A, list_for_each_entry_safe() remembers
> B as the alarm for the next loop iteration.
> 
> Now A re-schedules itself in 1 minute.  It is again the first entry in
> the alarm_list queue, but list_for_each_entry_safe() will not handle
> it in this loop.
> 
> Instead, do_alarm_run() sees B and returns "5 minutes" as the
> next_alarm time.
> 
> Solve this by breaking from the loop when a late event is detected,
> and letting get_next_alarm() do all the work.

Good point. I noticed this while rewriting the alarm scheduler, in the
rewrite it should handle this case correctly.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [nf-failover] [conntrack-utils PATCH r7254 08/14] check alarms even if select()!=0

[Pablo Neira Ayuso]

Max Kellermann wrote:
> It is not documented that select() always returns 0 when the timeout
> expires - the timeout may expire, and there may be events on file
> descriptors at the same time.  Don't check the select() return value,
> check timerisset(next_alarm) instead, and always update next_alarm
> with get_next_alarm()

Indeed, good point. This was fixed in r7260.

> Also the old code never checked for new alarms which were added during
> the file descriptor handlers.

Same thing.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [nf-failover] [conntrack-utils PATCH r7254 05/14] make the "tv" parameter const

[Pablo Neira Ayuso]

I have kept back 5/14, 6/14, 7/14 and 9/14 since I don't find an obvious
way to merge this to r7260. Thanks anyway for your work again.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers