On Mon, Jul 4, 2016 at 12:02 PM, Arne Schwabe <a...@rfc2549.org> wrote:
> >
> > Something like this is needed to protect against overflow of the 32
> > bit int (signed int in this case). Think of the user specifying sec =
> > 2^17. Any shift by more than 14 will make sec = 0. A more
> > sophisticated logic that counts the bits in sec before shifting is
> > possible, but this looked simple.. Now that a v3 is needed, will
> > change to an explicit check on overflow.
>
> That do that check in options.c and complain there. :)
Yes, we could limit n and m in "--connect-retry n [m]" to < 2^16 and then
limit the shift to 15 in init.c. But that adds dependency between two
statements in widely different locations. What I propose is to leave
options.c as is and change the exponential scaling code in init.c to
if (backoff > 0)
{
sec = max_int (sec, 1) << min_int (backoff, 31); /* left shifting int
by 32 or more is ill-defined */
if (sec == 0) /* overflow */
sec = INT_MAX;
}
No more confusing numbers like 16 and 15.
> + /* Slow down reconnection after 5 retries per remote */
> > + backoff = (c->options.unsuccessful_attempts /
> c->options.connection_list->len) - 4;
> > + if (backoff > 0 && sec < 1<<16)
> > + sec = max_int (sec, 1) << min_int (backoff, 15);
> > + if (sec > c->options.ce.connect_retry_seconds_max)
> > + sec = c->options.ce.connect_retry_seconds_max;
> > +
> > if (c->persist.restart_sleep_seconds > 0 &&
> c->persist.restart_sleep_seconds > sec)
> > sec = c->persist.restart_sleep_seconds;
> > else if (c->persist.restart_sleep_seconds == -1)
> >
> I think this also needs an excempt for TCP_SERVER case. I think moving
> the TCP_SERVER if below the exponential logic will work. Otherwise
> connecting just 10 times is an effect dos for a tcp server.
I am not sure I follow. Are you referring to the possibility that the sec =
1 for TCP_SERVER may get set to zero after 5 retries if "--connect-retry 0
0" is used? The server only listens for connections so this should not
happen, would it?.
Or do you have tcp-client in mind? In case of tcp-client, before this
patch, "--connect-retry 0" will hammer the server with repeated retries.
That will be avoided as max sec defaults to 300 sec, unless the user
specifies "--connect-retry 0 0". Shall we require that the second arg to
connect-retry should be greater than some minimum, say, at least 10 sec?
Selva
