Attention is currently required from: its_Giaan, plaisthos.

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

Change subject: Route: add support for user defined routing table
......................................................................


Patch Set 4: Code-Review-1

(1 comment)

Patchset:

PS4:
Sorry for being late to the party - I have looked at it, and while the general 
code works, I find it a bit excessive in lines of code.  Namely, I would opt 
for "do not support route-table as an extra argument to `route`", as this + the 
checks really makes up half the code - for little value.  Since it's not 
supposed to be pushable, routes-with-table need to be in the client config, and 
in that case, I could just put

```
route-table 123
route
route
route-table 456
route
route
route-table 999
# for everything that comes in pushed
```

into my config.  If I ever really need more than 1-2 route tables.

Also, we shouldn't have `#ifdef ENABLE_SITNL` in the parser for `route` and 
`route-ipv6`...

Further, the hunk

```
+    int table_id = 0; /* unspec table */
...
+    if (options->route_default_table_id)
+    {
+        table_id = options->route_default_table_id;
+    }
```

is sort of semi-useful - if route_default_table_id is 0, we do not assign it to 
table_id, which is also 0.  Why bother with an extra variable here that will 
have the same value as the `options` variable, at all times?  I'd just pass 
`options->route_default_table_id` downwards, and be done with it.

The `do_init_route_ipv6_list()` code also does the `table_id = ...` dance, but 
then passes NULL towards `add_route_ipv6_to_option_list()`, which does not look 
intentional...



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/524?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: I3e4ebef484d2a04a383a65ede5617ee98bf218a7
Gerrit-Change-Number: 524
Gerrit-PatchSet: 4
Gerrit-Owner: its_Giaan <gianma...@mandelbit.com>
Gerrit-Reviewer: cron2 <g...@greenie.muc.de>
Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com>
Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-open...@rfc2549.org>
Gerrit-Attention: its_Giaan <gianma...@mandelbit.com>
Gerrit-Comment-Date: Wed, 19 Jun 2024 09:13:14 +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