Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1253?usp=email )
Change subject: PUSH_UPDATE server: remove old IP(s) from vhash after sending a message containing ifconfig(-ipv6) ...................................................................... Patch Set 3: Code-Review-1 (7 comments) Patchset: PS3: I like this latest version, and it should work nicely. There might be a missing `free()` - this needs to be checked. While at it, I've found a few other things that could use a bit of polishing. File src/openvpn/multi.c: http://gerrit.openvpn.net/c/openvpn/+/1253/comment/723a899a_4779f06b?usp=email : PS3, Line 4296: msg(D_MULTI_LOW, "MULTI: Remove: %s -> %s", mroute_addr_print(&r->addr, &gc), multi_instance_string(mi, false, &gc)); this is too much logging ;-) I'd remove one of them, and change the other to include something like `MULTI: multi_unlearn_addr(): DEL %s` so it's clear it's not the normal "MULTI: DEL" path. Also, if we want to be totally fancy, we could use the mi prefix code here ``` set_prefix(mi); msg(...) // no multi_instance_string() needed, as that's handled by msg_set_prefix() clear_prefix(); ``` as for the message level, I'd use `D_MULTI_LOW` (as that is used by multi.c for the "Learn" message). http://gerrit.openvpn.net/c/openvpn/+/1253/comment/80047d20_27d7418e?usp=email : PS3, Line 4301: we might have a memleak here. I looked at the code in "multi.c" that deletes mroutes (check_stale_routes(), multi_reap_range()), and those call `multi_route_del(r)` to do ``` route_quota_dec(mi); multi_instance_dec_refcount(mi); free(route); ``` so you have the first two here, but not the `free()` - not 100% sure if this is required, but it smells like it. (`hash_remove_by_value()` seems to not do anything about the thing pointed to) Maybe just calling `multi_route_del(r)` here is the right thing (after the hash_remove_by_value() call, to avoid a use-after-free)? http://gerrit.openvpn.net/c/openvpn/+/1253/comment/f301c3e7_e77a6296?usp=email : PS3, Line 4320: ASSERT(mroute_extract_openvpn_sockaddr(&addr, &remote_si, false)); This works, but feels more clumsy than needed - looking through mroute.c, I find `mroute_get_in_addr_t(struct mroute_addr *out, const in_addr_t in, int mask)` which should do the same with less lines of intermediate structure filling :-) (have not tested this, just compared code) http://gerrit.openvpn.net/c/openvpn/+/1253/comment/8290fc03_3b80051e?usp=email : PS3, Line 4339: `mroute_get_in6_addr(&addr, a6, 0)` does this for you http://gerrit.openvpn.net/c/openvpn/+/1253/comment/cbc56eb6_41bab12b?usp=email : PS3, Line 4366: old_addr.s_addr = ntohl(mi->context.c2.push_ifconfig_local); we could just do `in_addr_t old_addr = ...` here and save the detour via `struct in_addr`? File src/openvpn/push_util.c: http://gerrit.openvpn.net/c/openvpn/+/1253/comment/07ab8769_ed9bfa83?usp=email : PS3, Line 158: memset(&o, 0, sizeof(struct options)); please use `CLEAR(o);` for that. It does exactly the same, but we use it in this situation so it's clear "this is not just a random memset() but to 0-initialize the options struct". -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1253?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: I07a8ddd9026eef64b6f5abde98702a9801616a5f Gerrit-Change-Number: 1253 Gerrit-PatchSet: 3 Gerrit-Owner: mrbff <[email protected]> Gerrit-Reviewer: cron2 <[email protected]> Gerrit-Reviewer: flichtenheld <[email protected]> Gerrit-Reviewer: plaisthos <[email protected]> Gerrit-CC: openvpn-devel <[email protected]> Gerrit-Attention: plaisthos <[email protected]> Gerrit-Attention: flichtenheld <[email protected]> Gerrit-Attention: mrbff <[email protected]> Gerrit-Comment-Date: Sun, 12 Oct 2025 11:18:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
