Hi,

The patch looks functionally ok.  Just one minor remark:

On 09-10-15 16:13, Lev Stipakov wrote:
@@ -103,10 +107,14 @@ multi_get_create_instance_udp (struct multi_context *m, 
bool *floated)
                      hash_add_fast (hash, bucket, &mi->real, hv, mi);
                      mi->did_real_hash = true;

+                     /* In future we might want to use P_DATA_V2 but not need 
peer-id/float functionality */
                      for (i = 0; i < m->max_clients; ++i)
                        {
                          if (!m->instances[i])
                            {
+                             /* issued peer-id should fit into 3 bytes to 
avoid wrap and cannot have reserved value 0xFFFFFF */
+                             ASSERT(i < 0xFFFFFF);
+
                              mi->context.c2.tls_multi->peer_id = i;
                              m->instances[i] = mi;
                              break;

Checking against the max peer-id indeed makes sense, but this assert can be made simpler by just putting

   ASSERT (m->max-clients < 0xFFFFFF);

before the loop. Additionally, I think we should add a user-friendly check in options.c, to give a proper error message on startup when a user tries to set --max-clients >= 0xFFFFFF.

Finally, why did you put this comment at this location? Shouldn't it be in the place where we *send* the peer-id? (That is the part that is not implemented now, right?)

-Steffan

Reply via email to