Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Gert D. <ge...@gr...>]

Hi,

On Sun, Nov 23, 2014 at 05:17:11PM +0200, Lev Stipakov wrote:
> Changes in v7:
> A few nitpicks.

Went in, and has just been pushed.  Time for your dance now :-)

Question to you:

> @@ -75,6 +101,16 @@ multi_get_create_instance_udp (struct multi_context *m)
>  		    {
>  		      hash_add_fast (hash, bucket, &mi->real, hv, mi);
>  		      mi->did_real_hash = true;
> +
> +		      for (i = 0; i < m->max_clients; ++i)
> +			{
> +			  if (!m->instances[i])
> +			    {
> +			      mi->context.c2.tls_multi->peer_id = i;
> +			      m->instances[i] = mi;
> +			      break;
> +			    }
> +			}

Is this code "safe"?  That is, if max_clients is set too low, and you have
more than this number of clients trying to connect, what will happen?

Will the server check this case before this loop, or will it just "not
find a free instance" and leave peer_id as "0"?

Maybe we should add an ASSERT() here, to be sure that whatever other pieces
of the code do, this will always end up in a defined state  (as a separate
patch).



Second, question to the group.  Right now, this code is active unconditionally
on the server side - what if someone does not want this behaviour, for
whatever reason?  Shall we make it depend on --float (which right now 
doesn't do anything for a --server OpenVPN server)?

This should be a fairly small change, I think, just never send the client
a "peer-id <n>" push-reply (so it won't use T_PACKET_V2), and check 
"if (options.float)" before checking whether it's a floated client...  
so the question is more along the line "is this a useful thing to do, 
nor not?".

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...