Hello,
Currently it should be safe, since multi_create_instance returns NULL
if amount of clients >= max_clients. In this case we won't reach that
"for" loop thanks to "if (mi)" check.
But it probably won't harm to assert if we've reached "for" loop and
could not find available "instance" item. I can implement that.
As for the second question - hard to say. If we make it opt-in, we
probably will need to announce this feature loudly to make users aware
of that. From the other side, it is not inconceivable to assume that
someone might not want this. Maybe we could make it opt-out and have
"-no-peer-id" config option?
-Lev
2014-11-27 16:22 GMT+02:00 Gert Doering <g...@greenie.muc.de>:
> 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 g...@greenie.muc.de
> fax: +49-89-35655025 g...@net.informatik.tu-muenchen.de
--
-Lev
