Menu
▾
▴
openvpn-devel
|
From: flichtenheld (C. Review) <ge...@op...> - 2025-10-27 13:08:36
|
Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1312?usp=email
to review the following change.
Change subject: multi: Fix type handling for hashes, mostly inotify_watchers
......................................................................
multi: Fix type handling for hashes, mostly inotify_watchers
Change-Id: Idede28c850def5e3b4a17dcbd0a5771f15cfc668
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M src/openvpn/multi.c
1 file changed, 15 insertions(+), 14 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/12/1312/1
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 74cc866..f9f6b16 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -268,7 +268,7 @@
*/
int_hash_function(const void *key, uint32_t iv)
{
- return (unsigned long)key;
+ return (uint32_t)(uintptr_t)key;
}
static bool
@@ -306,22 +306,23 @@
* to determine which client sent an incoming packet
* which is seen on the TCP/UDP socket.
*/
- m->hash = hash_init(t->options.real_hash_size, get_random(), mroute_addr_hash_function,
- mroute_addr_compare_function);
+ m->hash = hash_init(t->options.real_hash_size, (uint32_t)get_random(),
+ mroute_addr_hash_function, mroute_addr_compare_function);
/*
* Virtual address hash table. Used to determine
* which client to route a packet to.
*/
- m->vhash = hash_init(t->options.virtual_hash_size, get_random(), mroute_addr_hash_function,
- mroute_addr_compare_function);
+ m->vhash = hash_init(t->options.virtual_hash_size, (uint32_t)get_random(),
+ mroute_addr_hash_function, mroute_addr_compare_function);
/*
* This hash table is a clone of m->hash but with a
* bucket size of one so that it can be used
* for fast iteration through the list.
*/
- m->iter = hash_init(1, get_random(), mroute_addr_hash_function, mroute_addr_compare_function);
+ m->iter = hash_init(1, (uint32_t)get_random(), mroute_addr_hash_function,
+ mroute_addr_compare_function);
#ifdef ENABLE_MANAGEMENT
m->cid_hash = hash_init(t->options.real_hash_size, 0, cid_hash_function, cid_compare_function);
@@ -332,8 +333,8 @@
* Mapping between inotify watch descriptors and
* multi_instances.
*/
- m->inotify_watchers =
- hash_init(t->options.real_hash_size, get_random(), int_hash_function, int_compare_function);
+ m->inotify_watchers = hash_init(t->options.real_hash_size, (uint32_t)get_random(),
+ int_hash_function, int_compare_function);
#endif
/*
@@ -623,7 +624,7 @@
#ifdef ENABLE_ASYNC_PUSH
if (mi->inotify_watch != -1)
{
- hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch);
+ hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch);
mi->inotify_watch = -1;
}
#endif
@@ -2825,7 +2826,7 @@
msg(D_MULTI_DEBUG, "MULTI: modified fd %d, mask %d", pevent->wd, pevent->mask);
struct multi_instance *mi =
- hash_lookup(m->inotify_watchers, (void *)(unsigned long)pevent->wd);
+ hash_lookup(m->inotify_watchers, (void *)(uintptr_t)pevent->wd);
if (pevent->mask & IN_CLOSE_WRITE)
{
@@ -2844,7 +2845,7 @@
/* this event is _always_ fired when watch is removed or file is deleted */
if (mi)
{
- hash_remove(m->inotify_watchers, (void *)(unsigned long)pevent->wd);
+ hash_remove(m->inotify_watchers, (void *)(uintptr_t)pevent->wd);
mi->inotify_watch = -1;
}
}
@@ -2984,14 +2985,14 @@
const char *file)
{
/* watch acf file */
- long watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT);
+ int watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT);
if (watch_descriptor >= 0)
{
if (mi->inotify_watch != -1)
{
- hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch);
+ hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch);
}
- hash_add(m->inotify_watchers, (const uintptr_t *)watch_descriptor, mi, true);
+ hash_add(m->inotify_watchers, (void *)(uintptr_t)watch_descriptor, mi, true);
mi->inotify_watch = watch_descriptor;
}
else
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1312?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idede28c850def5e3b4a17dcbd0a5771f15cfc668
Gerrit-Change-Number: 1312
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-15 14:55:27
|
Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/1312?usp=email ) Change subject: multi: Fix type handling for hashes, mostly inotify_watchers ...................................................................... Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1312?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: Idede28c850def5e3b4a17dcbd0a5771f15cfc668 Gerrit-Change-Number: 1312 Gerrit-PatchSet: 9 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Mon, 15 Dec 2025 14:55:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes |
|
From: Gert D. <ge...@gr...> - 2025-12-15 14:55:36
|
From: Frank Lichtenheld <fr...@li...> Change-Id: Idede28c850def5e3b4a17dcbd0a5771f15cfc668 Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1312 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1312 This mail reflects revision 9 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index d9cb3a9..20d72c1 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -257,7 +257,7 @@ */ int_hash_function(const void *key, uint32_t iv) { - return (unsigned long)key; + return (uint32_t)(uintptr_t)key; } static bool @@ -295,22 +295,23 @@ * to determine which client sent an incoming packet * which is seen on the TCP/UDP socket. */ - m->hash = hash_init(t->options.real_hash_size, get_random(), mroute_addr_hash_function, - mroute_addr_compare_function); + m->hash = hash_init(t->options.real_hash_size, (uint32_t)get_random(), + mroute_addr_hash_function, mroute_addr_compare_function); /* * Virtual address hash table. Used to determine * which client to route a packet to. */ - m->vhash = hash_init(t->options.virtual_hash_size, get_random(), mroute_addr_hash_function, - mroute_addr_compare_function); + m->vhash = hash_init(t->options.virtual_hash_size, (uint32_t)get_random(), + mroute_addr_hash_function, mroute_addr_compare_function); /* * This hash table is a clone of m->hash but with a * bucket size of one so that it can be used * for fast iteration through the list. */ - m->iter = hash_init(1, get_random(), mroute_addr_hash_function, mroute_addr_compare_function); + m->iter = hash_init(1, (uint32_t)get_random(), mroute_addr_hash_function, + mroute_addr_compare_function); #ifdef ENABLE_MANAGEMENT m->cid_hash = hash_init(t->options.real_hash_size, 0, cid_hash_function, cid_compare_function); @@ -321,8 +322,8 @@ * Mapping between inotify watch descriptors and * multi_instances. */ - m->inotify_watchers = - hash_init(t->options.real_hash_size, get_random(), int_hash_function, int_compare_function); + m->inotify_watchers = hash_init(t->options.real_hash_size, (uint32_t)get_random(), + int_hash_function, int_compare_function); #endif /* @@ -609,7 +610,7 @@ #ifdef ENABLE_ASYNC_PUSH if (mi->inotify_watch != -1) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch); mi->inotify_watch = -1; } #endif @@ -2821,7 +2822,7 @@ msg(D_MULTI_DEBUG, "MULTI: modified fd %d, mask %d", pevent->wd, pevent->mask); struct multi_instance *mi = - hash_lookup(m->inotify_watchers, (void *)(unsigned long)pevent->wd); + hash_lookup(m->inotify_watchers, (void *)(uintptr_t)pevent->wd); if (pevent->mask & IN_CLOSE_WRITE) { @@ -2840,7 +2841,7 @@ /* this event is _always_ fired when watch is removed or file is deleted */ if (mi) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)pevent->wd); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)pevent->wd); mi->inotify_watch = -1; } } @@ -2978,14 +2979,14 @@ const char *file) { /* watch acf file */ - long watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT); + int watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT); if (watch_descriptor >= 0) { if (mi->inotify_watch != -1) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch); } - hash_add(m->inotify_watchers, (const uintptr_t *)watch_descriptor, mi, true); + hash_add(m->inotify_watchers, (void *)(uintptr_t)watch_descriptor, mi, true); mi->inotify_watch = watch_descriptor; } else |
|
From: Gert D. <ge...@gr...> - 2025-12-15 15:42:17
|
This code does not really cater to my sensitivities... but having a long
get_random() go into an uint32_t won't work without either some cast, or
adding a new get_random_u32()... which would be a bit overdoing things
as well (that said, looking at get_random()'s signedness, questions
do arise).
Anyway. The changes look reasonable. The hash stuff will not become
any more pretty - if a pointer is stored, but we really compare "integer
keys" (pointer-as-integer, not pointer-to-integer) casts are needed...
Not tested this beyond "BBs are happy", but the inotify stuff will be
excercised by the server side testbed (which will tell me tomorrow if
it *did* break something).
Your patch has been applied to the master branch.
commit e2c97f3833a5616004da4d84fb9e0d023c9033e5
Author: Frank Lichtenheld
Date: Mon Dec 15 15:55:23 2025 +0100
multi: Fix type handling for hashes, mostly inotify_watchers
Signed-off-by: Frank Lichtenheld <fr...@li...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1312
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg35072.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-15 15:42:12
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1312?usp=email ) Change subject: multi: Fix type handling for hashes, mostly inotify_watchers ...................................................................... multi: Fix type handling for hashes, mostly inotify_watchers Change-Id: Idede28c850def5e3b4a17dcbd0a5771f15cfc668 Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1312 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg35072.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/multi.c 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index d9cb3a9..20d72c1 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -257,7 +257,7 @@ */ int_hash_function(const void *key, uint32_t iv) { - return (unsigned long)key; + return (uint32_t)(uintptr_t)key; } static bool @@ -295,22 +295,23 @@ * to determine which client sent an incoming packet * which is seen on the TCP/UDP socket. */ - m->hash = hash_init(t->options.real_hash_size, get_random(), mroute_addr_hash_function, - mroute_addr_compare_function); + m->hash = hash_init(t->options.real_hash_size, (uint32_t)get_random(), + mroute_addr_hash_function, mroute_addr_compare_function); /* * Virtual address hash table. Used to determine * which client to route a packet to. */ - m->vhash = hash_init(t->options.virtual_hash_size, get_random(), mroute_addr_hash_function, - mroute_addr_compare_function); + m->vhash = hash_init(t->options.virtual_hash_size, (uint32_t)get_random(), + mroute_addr_hash_function, mroute_addr_compare_function); /* * This hash table is a clone of m->hash but with a * bucket size of one so that it can be used * for fast iteration through the list. */ - m->iter = hash_init(1, get_random(), mroute_addr_hash_function, mroute_addr_compare_function); + m->iter = hash_init(1, (uint32_t)get_random(), mroute_addr_hash_function, + mroute_addr_compare_function); #ifdef ENABLE_MANAGEMENT m->cid_hash = hash_init(t->options.real_hash_size, 0, cid_hash_function, cid_compare_function); @@ -321,8 +322,8 @@ * Mapping between inotify watch descriptors and * multi_instances. */ - m->inotify_watchers = - hash_init(t->options.real_hash_size, get_random(), int_hash_function, int_compare_function); + m->inotify_watchers = hash_init(t->options.real_hash_size, (uint32_t)get_random(), + int_hash_function, int_compare_function); #endif /* @@ -609,7 +610,7 @@ #ifdef ENABLE_ASYNC_PUSH if (mi->inotify_watch != -1) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch); mi->inotify_watch = -1; } #endif @@ -2821,7 +2822,7 @@ msg(D_MULTI_DEBUG, "MULTI: modified fd %d, mask %d", pevent->wd, pevent->mask); struct multi_instance *mi = - hash_lookup(m->inotify_watchers, (void *)(unsigned long)pevent->wd); + hash_lookup(m->inotify_watchers, (void *)(uintptr_t)pevent->wd); if (pevent->mask & IN_CLOSE_WRITE) { @@ -2840,7 +2841,7 @@ /* this event is _always_ fired when watch is removed or file is deleted */ if (mi) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)pevent->wd); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)pevent->wd); mi->inotify_watch = -1; } } @@ -2978,14 +2979,14 @@ const char *file) { /* watch acf file */ - long watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT); + int watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT); if (watch_descriptor >= 0) { if (mi->inotify_watch != -1) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch); } - hash_add(m->inotify_watchers, (const uintptr_t *)watch_descriptor, mi, true); + hash_add(m->inotify_watchers, (void *)(uintptr_t)watch_descriptor, mi, true); mi->inotify_watch = watch_descriptor; } else -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1312?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idede28c850def5e3b4a17dcbd0a5771f15cfc668 Gerrit-Change-Number: 1312 Gerrit-PatchSet: 10 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> |
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-15 15:42:16
|
cron2 has uploaded a new patch set (#10) to the change originally created by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/1312?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: multi: Fix type handling for hashes, mostly inotify_watchers ...................................................................... multi: Fix type handling for hashes, mostly inotify_watchers Change-Id: Idede28c850def5e3b4a17dcbd0a5771f15cfc668 Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1312 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg35072.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/multi.c 1 file changed, 15 insertions(+), 14 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/12/1312/10 diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index d9cb3a9..20d72c1 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -257,7 +257,7 @@ */ int_hash_function(const void *key, uint32_t iv) { - return (unsigned long)key; + return (uint32_t)(uintptr_t)key; } static bool @@ -295,22 +295,23 @@ * to determine which client sent an incoming packet * which is seen on the TCP/UDP socket. */ - m->hash = hash_init(t->options.real_hash_size, get_random(), mroute_addr_hash_function, - mroute_addr_compare_function); + m->hash = hash_init(t->options.real_hash_size, (uint32_t)get_random(), + mroute_addr_hash_function, mroute_addr_compare_function); /* * Virtual address hash table. Used to determine * which client to route a packet to. */ - m->vhash = hash_init(t->options.virtual_hash_size, get_random(), mroute_addr_hash_function, - mroute_addr_compare_function); + m->vhash = hash_init(t->options.virtual_hash_size, (uint32_t)get_random(), + mroute_addr_hash_function, mroute_addr_compare_function); /* * This hash table is a clone of m->hash but with a * bucket size of one so that it can be used * for fast iteration through the list. */ - m->iter = hash_init(1, get_random(), mroute_addr_hash_function, mroute_addr_compare_function); + m->iter = hash_init(1, (uint32_t)get_random(), mroute_addr_hash_function, + mroute_addr_compare_function); #ifdef ENABLE_MANAGEMENT m->cid_hash = hash_init(t->options.real_hash_size, 0, cid_hash_function, cid_compare_function); @@ -321,8 +322,8 @@ * Mapping between inotify watch descriptors and * multi_instances. */ - m->inotify_watchers = - hash_init(t->options.real_hash_size, get_random(), int_hash_function, int_compare_function); + m->inotify_watchers = hash_init(t->options.real_hash_size, (uint32_t)get_random(), + int_hash_function, int_compare_function); #endif /* @@ -609,7 +610,7 @@ #ifdef ENABLE_ASYNC_PUSH if (mi->inotify_watch != -1) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch); mi->inotify_watch = -1; } #endif @@ -2821,7 +2822,7 @@ msg(D_MULTI_DEBUG, "MULTI: modified fd %d, mask %d", pevent->wd, pevent->mask); struct multi_instance *mi = - hash_lookup(m->inotify_watchers, (void *)(unsigned long)pevent->wd); + hash_lookup(m->inotify_watchers, (void *)(uintptr_t)pevent->wd); if (pevent->mask & IN_CLOSE_WRITE) { @@ -2840,7 +2841,7 @@ /* this event is _always_ fired when watch is removed or file is deleted */ if (mi) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)pevent->wd); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)pevent->wd); mi->inotify_watch = -1; } } @@ -2978,14 +2979,14 @@ const char *file) { /* watch acf file */ - long watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT); + int watch_descriptor = inotify_add_watch(inotify_fd, file, IN_CLOSE_WRITE | IN_ONESHOT); if (watch_descriptor >= 0) { if (mi->inotify_watch != -1) { - hash_remove(m->inotify_watchers, (void *)(unsigned long)mi->inotify_watch); + hash_remove(m->inotify_watchers, (void *)(uintptr_t)mi->inotify_watch); } - hash_add(m->inotify_watchers, (const uintptr_t *)watch_descriptor, mi, true); + hash_add(m->inotify_watchers, (void *)(uintptr_t)watch_descriptor, mi, true); mi->inotify_watch = watch_descriptor; } else -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1312?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idede28c850def5e3b4a17dcbd0a5771f15cfc668 Gerrit-Change-Number: 1312 Gerrit-PatchSet: 10 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> |
Thanks for helping keep SourceForge clean.
X