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 <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-Comment-Date: Mon, 21 Jul 2025 18:25:37 +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