Attention is currently required from: flichtenheld, plaisthos.

its_Giaan has posted comments on this change by its_Giaan. ( 
http://gerrit.openvpn.net/c/openvpn/+/1521?usp=email )

Change subject: Add support for user defined network namespace
......................................................................


Patch Set 2:

(24 comments)

File src/openvpn/dco.c:

http://gerrit.openvpn.net/c/openvpn/+/1521/comment/896aa530_aee37115?usp=email :
PS1, Line 394:         msg(msglevel, "Note: --netns not supported by ovpn-dco, 
disabling data channel offload.");
> `ovpn` ? Or just "DCO"?
Done


File src/openvpn/networking_sitnl.h:

http://gerrit.openvpn.net/c/openvpn/+/1521/comment/54e1d744_317f8b7a?usp=email :
PS1, Line 52:  *              NETNS_RUN_DIR, e.g. /var/run/netns/<name>).
> Doxygen says `networking_sitnl. […]
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/dcdf2078_aa274ea0?usp=email :
PS1, Line 98: int openvpn_if_nametoindex(const char *ifname, int netnsid);
> This is ALWAYS called with `get_or_create_netnsid_sitnl`, so just move all of 
> that code inside and c […]
I'm not thrilled by the idea of having a function that does too much stuff so I 
only moved the call to get_or_create_netnsid_sitnl() inside.


File src/openvpn/networking_sitnl.c:

http://gerrit.openvpn.net/c/openvpn/+/1521/comment/fa0f2215_3cf98d28?usp=email :
PS1, Line 74: #define NETNS_RUN_DIR  "/var/run/netns"
> Wouldn't `/run` be a better value here? `/var/run` is just a symlink nowadays 
> AFAIK
indeed


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/f4d47f64_4f660ee1?usp=email :
PS1, Line 206:             msg(M_WARN, "%s: Network namespace %s does not 
exist\n", __func__, path);
> M_ERRNO?
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/c43227fc_0359781b?usp=email :
PS1, Line 239:         msg(M_WARN, "%s: Cannot open original network 
namespace\n", __func__);
> M_ERRNO?
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/67a2f3a1_46be9598?usp=email :
PS1, Line 247:         msg(M_WARN, "%s: Cannot open network namespace 
\"%s\"\n", __func__, name);
> M_ERRNO
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/be497947_bd26b285?usp=email :
PS1, Line 254:         msg(M_WARN, "%s: setting the network namespace \"%s\" 
failed\n", __func__, name);
> M_ERRNO
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/4c35413c_6640d246?usp=email :
PS1, Line 272:         msg(M_WARN, "%s: Cannot restore original network 
namespace\n", __func__);
> M_ERRNO
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/742c386b_4519cc6f?usp=email :
PS1, Line 635:         goto err;
> just use return
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/22e23784_c162668b?usp=email :
PS1, Line 841:         goto err;
> M_ERR is fatal
M_ERRNO?


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/8a284417_909fc242?usp=email :
PS1, Line 848:         goto err;
> M_ERR is fatal
M_ERRNO?


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/d3926303_567f83d5?usp=email :
PS1, Line 914:     if (netns)
> These 4 lines (counting the netns assignment) are copied often enough that 
> they should be moved to a […]
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/88485286_b7b56c97?usp=email :
PS1, Line 928:         msg(M_WARN, "%s: rtnl: cannot get ifindex for %s: %s", 
__func__, iface, strerror(errno));
> Should use M_ERRNO instead of manually using strerror
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/f5ac3d5e_ae177443?usp=email :
PS1, Line 950:     ret = sitnl_send(&req.n, 0, 0, NULL, NULL);
> Just declare ret here. C has moved on...
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/18e6b8dd_66e39679?usp=email :
PS1, Line 953:         goto err;
> Just use return
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/9305239d_8149d39f?usp=email :
PS1, Line 955:     if (orig >= 0)
> Also move to function or macro
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/43479fdf_4052fd95?usp=email :
PS1, Line 1002: err:
> Unused label?
SITNL_ADDATTR() requires a label


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/164779a4_0a0706ba?usp=email :
PS1, Line 1019:
> spurious empty line
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/034b03f7_9d56a11c?usp=email :
PS1, Line 1064:
> spurious empty line
Done


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/c991facb_65fb1024?usp=email :
PS1, Line 1124: err:
> Unused label
SITNL_ADDATTR() requires a label


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/9da49ebb_0c7cdb54?usp=email :
PS1, Line 1833:         goto err;
> WHAT?
Whoops


http://gerrit.openvpn.net/c/openvpn/+/1521/comment/a803d839_9934efff?usp=email :
PS1, Line 1881:         goto err;
> lol
Done


File src/openvpn/options.c:

http://gerrit.openvpn.net/c/openvpn/+/1521/comment/2b61fd19_da804455?usp=email :
PS1, Line 6476: #if defined(TARGET_LINUX)
> The documentation says it is a no-op on non-Linux, but as you implemented it 
> is an error I think?
indeed



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1521?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: I8b0d1cad7062856abcc40c4e16ec93b45295bbd3
Gerrit-Change-Number: 1521
Gerrit-PatchSet: 2
Gerrit-Owner: its_Giaan <[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-Comment-Date: Thu, 26 Feb 2026 09:48:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to