Hi,

On Mon, Jul 4, 2016 at 5:23 AM, Arne Schwabe <a...@rfc2549.org> wrote:

>  - sndbuf and recvbuf default now to OS default instead of 64k
> >
> > @@ -120,6 +120,10 @@ User-visible Changes
> >  - --http-proxy-retry and --sock-proxy-retry have been removed. Proxy
> connections
> >      will now behave like regular connection entries and generate a USR1
> on failure.
> >
> > +- --connect-retry gets a optional third argument that specifies the
> maximum
> > +  time in seconds to wait between reconnection attempts when an
> exponential
> > +  backoff is triggered due to repeated retries. Default = 300 seconds.
> Shouldn't that be 2nd argument?


Ugh... should be 2nd. Will do a v3..

> +  backoff = (c->options.unsuccessful_attempts /
> c->options.connection_list->len) - 4;
> > +  if (backoff > 0 && sec < 1<<16)
> the 1<<16 seems a bit random here. Could you at least add a comment of
> why? I don't see any valid reason here since sec is limited between 1 and
> connect_retry_seconds_max by the next two lines anyway.
> > +    sec = max_int (sec, 1) << min_int (backoff, 15);
> The 15 here is the same. Both lines seem to only make sense with 16 bit
> integers.
>

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.

> +  if (sec > c->options.ce.connect_retry_seconds_max)
> > +    sec = c->options.ce.connect_retry_seconds_max;
> Oh, my suggestion made the max time time configurable per connection
> entry. We should address that in a seperate patch. I think connect-retry
> as a whole can be made a non connection config entry. (connect-retry
> traditionally was the time between tcp connects for a tcp connection
> which looped on its own before the dual stack patch if anyone wonders
> why it was connection specific).


It can still be specified as a common number outside connection entries, so
leaving it as a connection-specific number looks fine to me.

Selva

Reply via email to