Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change by plaisthos. ( 
http://gerrit.openvpn.net/c/openvpn/+/1727?usp=email )

Change subject: Add lookup of multi session by session id
......................................................................


Patch Set 3: Code-Review-2

(10 comments)

File doc/man-sections/advanced-options.rst:

http://gerrit.openvpn.net/c/openvpn/+/1727/comment/063287a6_ad77a9f8?usp=email :
PS3, Line 32:   address table to ``v``. And if specified the session id hash 
table to
"If specified set the size of the session id hash table to"


http://gerrit.openvpn.net/c/openvpn/+/1727/comment/adc8d07e_8b554a73?usp=email :
PS3, Line 33:   ``s``. Otherwise ``s`` be set to the same value as ``r``.
"will be"


File src/openvpn/mudp.c:

http://gerrit.openvpn.net/c/openvpn/+/1727/comment/d3040fdc_2606f6b9?usp=email :
PS3, Line 383:         if (m->top.c2.buf.len < 9)
Why hardcode 9 instead of "sizeof(struct session_id) + 1" ?


File src/openvpn/multi.h:

http://gerrit.openvpn.net/c/openvpn/+/1727/comment/f186bf4c_f7a8e8df?usp=email :
PS3, Line 138:      * id here is non-null and the hash map's key pointer uses 
is pointing
Not sure what "uses" is supposed to mean here?


http://gerrit.openvpn.net/c/openvpn/+/1727/comment/4df36e1e_1916a232?usp=email :
PS3, Line 139:      * here */
What is "here"? Sounds like it points to this part of the struct, but it 
actually points to the beginning of the struct, doesn't it?


http://gerrit.openvpn.net/c/openvpn/+/1727/comment/2fd8e60c_76963cb3?usp=email :
PS3, Line 178:                                             session id. We do 
not care collisions here
"care about"


File src/openvpn/multi.c:

http://gerrit.openvpn.net/c/openvpn/+/1727/comment/ad114933_bf7dfcce?usp=email :
PS3, Line 441:         if (mi->context.c2.tls_multi && true)
"&& true" ?


File src/openvpn/sid_hash.h:

http://gerrit.openvpn.net/c/openvpn/+/1727/comment/30682dc5_7bd0156f?usp=email :
PS3, Line 35:     /* This only be called if the multi instance is not already 
present
"This should" or "This must"?


http://gerrit.openvpn.net/c/openvpn/+/1727/comment/1001dcdb_16f4167a?usp=email :
PS3, Line 52:     struct hash_element *he = hash_lookup_fast(m->sid_hash, 
bucket, sid, hv);
Could reduce code duplication slightly by using multi_hash_sid_lookup here.


File tests/unit_tests/openvpn/test_misc.c:

http://gerrit.openvpn.net/c/openvpn/+/1727/comment/c82c40ee_2c2abdc4?usp=email :
PS3, Line 143: get_random(void)
```
test_misc.c: In function ‘test_list’:
test_misc.c:258:19: error: conversion from ‘long int’ to ‘uint32_t’ {aka 
‘unsigned int’} may change value [-Werror=conversion]
  258 |             inc = (get_random() % 3) + 1;
      |                   ^
```



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1727?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idb59ecd119331b198792ad1379bec8600211651b
Gerrit-Change-Number: 1727
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Comment-Date: Mon, 06 Jul 2026 12:47:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to