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

Reply via email to