[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE: Added update_option() function.
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/810?usp=email ) Change subject: PUSH_UPDATE: Added update_option() function. .. Patch Set 21: Code-Review-1 (5 comments) Patchset: PS21: Nagging about code duplication... File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/810/comment/e80b547e_689dbd50 : PS21, Line 5897: } I find this duplication of syntax checking, mmmh, sub-optimal. We'll check the syntax anyway in `add_option()` later on... I could see user complaints about "I sent a PUSH_UPDATE with one invalid `route` in it, and all my routes were lost", but honestly, do we really care that much? (If yes, we could consider moving the syntax check into a small helper, if not, just drop the lines from `rol_check_alloc()` up to this line). http://gerrit.openvpn.net/c/openvpn/+/810/comment/e1f5b5bd_f34b1d7f : PS21, Line 5927: } same here http://gerrit.openvpn.net/c/openvpn/+/810/comment/4054ed13_3dac7037 : PS21, Line 6009: same here... lots of code that needs to be in sync with the add_option() checks... http://gerrit.openvpn.net/c/openvpn/+/810/comment/abc555fa_e0e5856c : PS21, Line 6045: #endif I note a distinct lack of syntax checking for `dhcp-option` ;-) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/810?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: Ia45c99e6df7b3ad24020c10b8a9b3577984ecdc2 Gerrit-Change-Number: 810 Gerrit-PatchSet: 21 Gerrit-Owner: mrbff Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: stipa Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: flichtenheld Gerrit-Attention: mrbff Gerrit-Comment-Date: Mon, 21 Jul 2025 19:21:15 + 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
[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE message sender: enabling the server to send PUSH_UPDATE c...
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 Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: stipa Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: flichtenheld Gerrit-Attention: mrbff Gerrit-Attention: stipa Gerrit-Comment-Date: Mon, 21 Jul 2025 19:5
[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH ...
Attention is currently required from: cron2, flichtenheld, mrbff, stipa.
cron2 has posted comments on this change. (
http://gerrit.openvpn.net/c/openvpn/+/808?usp=email )
Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle
PUSH UPDATE control messages to allow options updating at runtime.
..
Patch Set 19: Code-Review-1
Copied votes on follow-up patch sets have been updated:
* Code-Review-1 has been copied to patch set 20 (copy condition:
"changekind:NO_CHANGE OR changekind:TRIVIAL_REBASE OR is:MIN").
(10 comments)
Patchset:
PS19:
So, I think this works and is sufficently save and efficient, but I'm and old
man and like my lawn orderly... so as discussed on IRC, please adjust a few
minor points.
File src/openvpn/options.c:
http://gerrit.openvpn.net/c/openvpn/+/808/comment/849fb9c1_8109d087 :
PS19, Line 5567: apply_push_options(struct options *options,
please do not move that one around in options.c, this makes comparing old/new
harder without good benefit, also breaks git blame...
http://gerrit.openvpn.net/c/openvpn/+/808/comment/25512e6f_b2eb183f :
PS19, Line 5654:
so I would call check_push_update_option_flags() from here, possibly moving the
"while isspace(*line) { line++; }" part up here as well. Then we can keep
"apply_pull_filter()" focused on "pull filter", not dealing with a number of
extra conditions... (just "is_update", see there)
File src/openvpn/options_util.c:
http://gerrit.openvpn.net/c/openvpn/+/808/comment/5c5cd074_1394fe2c :
PS19, Line 198: if (!line || !*line || !**line)
the use of a double pointer irritates me, because we never move the pointer,
instead `memmove()` the buffer...
I think we should just do `(*line)++` instead of the `memmove()` below -
possibly also getting rid of the `strlen(str)` thing, or `str` altogether :-) -
so keep the pointer, call it from `apply_push_options()` and accept a moving
`*line` pointer.
Also, I'm not sure if "empty string" should be treated as `false` or just
ignored (return true, that is) - the existing environment where
`apply_pull_filter()` is used does not care for "empty lines".
A NULL pointer is a programming error that must never happen, so an
ASSERT(line) is valid.
http://gerrit.openvpn.net/c/openvpn/+/808/comment/6ffca9a9_893ed859 :
PS19, Line 231:
If there is whitespace here (`? route`) we'll fail with `pushed option is not
updateable: ' route'`, if I'm not misreading it.
I'd document that "there must not be whitespace between the flag letters and
the option" and then we could add an `if (isspace(**line)) { complain; return
false; }` here... the documentation is important, the error handling not so
much (as it will hit the `return false` anyway)
http://gerrit.openvpn.net/c/openvpn/+/808/comment/bc8b42ab_3cf0ba0c :
PS19, Line 252: msg(D_PUSH, "Pushed option is not updatable: '%s'",
*line);
Maybe add an " Ignoring." to the string, so it's clear "this is informational,
but not an error, and this is how we handle it"? Since the other one has
"Restarting"...
http://gerrit.openvpn.net/c/openvpn/+/808/comment/ce24fecc_c0a39fcd :
PS19, Line 270: if (!o->pull_filter_list && !(is_update))
So, if we call `check_push_update_option_flags()` from outside, we do not need
this extra dance, checking o->pull_filter_list twice...
http://gerrit.openvpn.net/c/openvpn/+/808/comment/8974bb49_5a803112 :
PS19, Line 276: while (isspace(*line))
It makes sense to move that upwards to `apply_push_options()`, before calling
anything on the line. So both all functions called do not need to bother about
leading whitespace.
http://gerrit.openvpn.net/c/openvpn/+/808/comment/8689afde_5d9ee877 :
PS19, Line 310: return true;
This duplication of the messages and `return true` looks a bit silly :-) - I
would suggest to do
```
/* on PUSH_UPDATE, "reject" and "ignore" filters are treated the same */
else if ( (f->type == PUF_TYPE_IGNORE || is_update)
&& strncmp(line, f->pattern, f->size) == 0)
{
msg(D_PUSH, "Pushed option removed by filter: '%s'", line);
return true;
}
```
File src/openvpn/push_util.c:
http://gerrit.openvpn.net/c/openvpn/+/808/comment/b8dd5363_84e249de :
PS19, Line 23:true))
Just for the record - this is the really-old-style OpenVPN formatting, which we
no longer use. That said, I know it's the same as in
`process_incoming_push_reply()`, so it makes sense to keep it the same, and
then clang-format will change both at once.
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?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: I6ecd4cb47571cc8c20e46de8595c742aeec6064a
Gerrit-Change-Number: 808
Gerrit-PatchSet: 19
Gerrit-Owner: mrbff
Ger
[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE: Added remove_option() and do_update().
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/809?usp=email ) Change subject: PUSH_UPDATE: Added remove_option() and do_update(). .. Patch Set 21: Code-Review-1 (4 comments) Patchset: PS21: As discussed on IRC, I do have some things I'd like to see changed before merging. I am taking note of the +2's already given, it's just polishing, comments, readability... File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/20d8d7ae_599aaf68 : PS21, Line 2833: if (!is_update && !check_pull_client_ncp(c, found)) does it make sense to suppress this check? It should never fail, and never modify anything (as we passed on the first run). As it is, this code is a bit confusing, so we'd need a comment ``` /* on PUSH_UPDATE, do not run the NCP checks, because they take 5 minutes * to complete and we are in a hurry */ ``` (put the real reasoning there, of course ;-) ) Mmmh, I think I see the reason, OPT_P_NCP is not set on PUSH_UPDATE, so the code assumes "we have no cipher" and does fallback things... so the comment would need to be something like ``` /* on PUSH_UPDATE, NCP related flags are never updated, and so the code * would assume "no cipher pushed = NCP failed" - so, don't call it on * updates */ ``` File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/809/comment/758d81ff_703882a6 : PS21, Line 5502: options_server_import(struct options *o, please do not move them (as for 808). thanks :-) http://gerrit.openvpn.net/c/openvpn/+/809/comment/dbc0b2a7_f3d837dd : PS21, Line 3134: } with the extra code, the comment before the function is no longer appropriate. Also, it's unclear to me why this is needed? I'm not guessing the reasoning but claiming "this does not look reasonable" - if `redirect-gateway def1` was pushed or updated, we should not remove it again just because it's not METHOD_SERVICE? If we need it, we definitely need a comment to explain why. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?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: I507180d7397b6959844a30908010132bc3411067 Gerrit-Change-Number: 809 Gerrit-PatchSet: 21 Gerrit-Owner: mrbff Gerrit-Reviewer: cron2 Gerrit-Reviewer: flichtenheld Gerrit-Reviewer: plaisthos Gerrit-Reviewer: stipa Gerrit-CC: openvpn-devel Gerrit-Attention: plaisthos Gerrit-Attention: flichtenheld Gerrit-Attention: mrbff Gerrit-Comment-Date: Mon, 21 Jul 2025 18:25:37 + 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
