Attention is currently required from: flichtenheld, mrbff, plaisthos, stipa.
cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/869?usp=email ) Change subject: PUSH_UPDATE message sender: enabling the server to send PUSH_UPDATE control messages ...................................................................... Patch Set 16: Code-Review-1 (13 comments) Patchset: PS16: lots of string and buffer related nagging, plus some general questioning if we need this in the full breadth... File doc/management-notes.txt: http://gerrit.openvpn.net/c/openvpn/+/869/comment/a8274ef9_1e452d98 : PS16, Line 1075: I do question whether this is a reasonable approach, to have "by cn" and "by addr" functions, that add code (... that needs to be tested and maintained) when a management client can just look up the CID itself, and then do `push-update-cid`? Is this something AS wants? Or "let's just do all variants, so nobody complains about a missing feature"? File src/openvpn/manage.c: http://gerrit.openvpn.net/c/openvpn/+/869/comment/54080c2e_539d2c6e : PS16, Line 1362: } I bet there are more elegant and less repetitive ways to do 4 different `if (status) print(SUCCESS!) else print(ERROR)` blocks... File src/openvpn/push.h: http://gerrit.openvpn.net/c/openvpn/+/869/comment/f1e00c73_49d8e6c7 : PS16, Line 54: This does confuse me a bit. If we have no ENABLE_MANAGEMENT, why would we have UPD_BY_ADDR or UPD_BY_CN? Is this usable from a plugin interface or a script? File src/openvpn/push_util.c: http://gerrit.openvpn.net/c/openvpn/+/869/comment/fb47d1a4_53787e8b : PS16, Line 66: /* Allocate memory and asseble the final message */ typo http://gerrit.openvpn.net/c/openvpn/+/869/comment/384c531b_cb1d0b7b : PS16, Line 82: } it might increase readability to juse use `snprintf( ... "%s,%s%s", push_update_cmd, src, continuation? continuation: 0)` here... and get rid of `i`. Or use `buf_printf()` and get rid of the length and malloc stuff... (returning `BSTR(&buf)` then). http://gerrit.openvpn.net/c/openvpn/+/869/comment/e1cd130c_c8e39305 : PS16, Line 122: msgs[im] = forge_msg(str, ",push-continuation 2", gc); I do wonder if this can be done without modifying the string, so getting rid of the `strdup()` requirement... http://gerrit.openvpn.net/c/openvpn/+/869/comment/5e9e0424_e91519f2 : PS16, Line 143: /* It actually send the already divided messagge to one single client */ This comment needs a visit from the grammar police ;-) ``` /* send the message(s) prepared to one single client */ ``` http://gerrit.openvpn.net/c/openvpn/+/869/comment/648be3fc_6ea876c8 : PS16, Line 158: buf_write(&buf, msgs[i], strlen(msgs[i])); If you want to have the message in a `buf` here, you could have `forge_msg()` just use `buf_printf()` and return the resulting `buf`...? http://gerrit.openvpn.net/c/openvpn/+/869/comment/caf4cb50_9100e63f : PS16, Line 165: /* After sending the control message, we update the options server-side in the client's context */ Can you extend the comment a bit on why this is needed? I assume (which might or might not be a bad idea) that this is so `ifconfig-push` and `ifconfig-ipv6-push` can work? `cipher` etc. is not PUSH_UPDATE-able, and most other options the server should not care about? But without a bit of more specific explanation, this is very, uh, non-intuitive stuff. http://gerrit.openvpn.net/c/openvpn/+/869/comment/693c5314_a1a08fa4 : PS16, Line 204: if (!message_splitter(gc_strdup(msg, &gc), msgs, &gc, safe_cap)) Please do not call `strdup()` in a function call... yes, this works, but it is hard to read and no gain compared to "just call strdup() at the beginning of `message_splutter()` (if it turns out to be necessary). http://gerrit.openvpn.net/c/openvpn/+/869/comment/8aa9f5e8_3e0aac7c : PS16, Line 239: how can we ever end here if ENABLE_MANAGEMENT is not defined? Except for the unit test, there is no caller remaining... http://gerrit.openvpn.net/c/openvpn/+/869/comment/ecc7ac6a_56bfc11e : PS16, Line 305: RETURN_UPDATE_STATUS(n_sent); so here we have this nice if/else macro that manage.c could use :-)... but only 3 out of 4 functions use it? Can we not do the "does this CID exist?" check in the caller (for `management_callback_send_push_update_by_cid()`)? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/869?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ie82bcc7a8e583de9156b185d71d1a323ed8df3fc Gerrit-Change-Number: 869 Gerrit-PatchSet: 16 Gerrit-Owner: mrbff <ma...@mandelbit.com> Gerrit-Reviewer: cron2 <g...@greenie.muc.de> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org> Gerrit-Reviewer: stipa <lstipa...@gmail.com> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Attention: mrbff <ma...@mandelbit.com> Gerrit-Attention: stipa <lstipa...@gmail.com> Gerrit-Comment-Date: Mon, 21 Jul 2025 19:53:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
