[Openvpn-devel] [M] Change in openvpn[master]: Route: add support for user defined routing table

[cron2 (C. Review) <ge...@op...>]

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 <gia...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: its_Giaan <gia...@ma...>
Gerrit-Comment-Date: Wed, 19 Jun 2024 09:13:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment