| 14 | |
| 15 | == SVN merger == |
| 16 | |
| 17 | Dazo made a [http://openvpn.git.sourceforge.net/git/gitweb.cgi?p=openvpn/openvpn-testing.git;a=commitdiff;h=e47fb603ed721bb718495e6f8ed42ec134da2f98 heroic merge] of James' SVN branch to "master". We need to discuss this in more detail. Here are comments from cron2: |
| 18 | |
| 19 | {{{ |
| 20 | |
| 21 | Random ramblings in the order I go through things... |
| 22 | |
| 23 | - I git-clone'd openvpn-testing.git, went to the svn-merger branch, and |
| 24 | ran "make check" (on Gentoo Linux), with my full-featured t_client.rc |
| 25 | setup. |
| 26 | |
| 27 | Test ran: |
| 28 | - p2mp tun udp (ipv4 + ipv6 ok) |
| 29 | - p2mp tun tcp (ipv4 + ipv6 ok) |
| 30 | - p2mp tun udp, "topology subnet" (ipv4 + ipv6 ok) |
| 31 | - p2mp tap udp (ipv4 ok, ipv6 fails, known issue with IPv6 auto-conf |
| 32 | on TAP, not related to the svn-merger) |
| 33 | |
| 34 | so the client code (at least) is still working as well as my tests |
| 35 | cover the code. |
| 36 | |
| 37 | - code alignment needed: for IPv4, the "did_redirect_default_gateway" |
| 38 | and "spec.remote_endpoint_defined" have been converted to flag bits in |
| 39 | route_list-iflags, but for IPv6, the old structure elements |
| 40 | remain - so to make the code more "in-line" for IPv4 and IPv6, this |
| 41 | needs code adjustments in the IPv6 code. |
| 42 | |
| 43 | - I'm not overly happy about the "default-gateway block-local" changes - |
| 44 | this is less a code issue (the code might be fine) but a procedural |
| 45 | issue, with a huge change to route.c coming in without any sort of |
| 46 | review or discussion. Gah. (No response needed). |
| 47 | |
| 48 | - I'm somewhat more annoyed by this one (route.c, line 1280): |
| 49 | |
| 50 | #if defined(TARGET_LINUX) |
| 51 | #ifdef CONFIG_FEATURE_IPROUTE |
| 52 | /* FIXME -- add LR_MATCH support for CONFIG_FEATURE_IPROUTE */ |
| 53 | |
| 54 | this is implemented only for the "non-iproute2" case, so we have |
| 55 | differing behaviour for iproute2/non-iproute2 compiles now. |
| 56 | This MUST be fixed for 2.3 |
| 57 | |
| 58 | - implementations of LR_MATCH for most other platforms are missing, |
| 59 | but this is something that can be documented in the release notes, |
| 60 | and if someone thinks they need this, they can add it - but having |
| 61 | support-or-not for Linux, depending on --enable-iproute2, is a no-go |
| 62 | |
| 63 | - the merger has PF_INET6 blocks, and I currently don't run tests over |
| 64 | IPv6 transport. So maybe jjo could also take a look at this branch |
| 65 | and see whether his stuff is still working. |
| 66 | |
| 67 | - the web view of route.c, "-887,13 - +894,10" looks a bit weird, |
| 68 | with "++add_routes (...", but the code in the branch is fine. |
| 69 | |
| 70 | - there's a functional and potentially-fatal change here: |
| 71 | |
| 72 | void |
| 73 | -delete_routes (struct route_list *rl, const struct tuntap *tt, unsigned int flags, const struct env_set *es) |
| 74 | +delete_routes (struct route_list *rl, struct route_ipv6_list *rl6, |
| 75 | + const struct tuntap *tt, unsigned int flags, const struct env_set *es) |
| 76 | { |
| 77 | - if (rl&& rl-routes_added) |
| 78 | + if (rl-iflags& RL_ROUTES_ADDED) |
| 79 | |
| 80 | |
| 81 | this new code does not check whether "rl" is non-NULL, but in theory |
| 82 | it could very well be NULL if we only have IPv6 routes. |
| 83 | |
| 84 | So route.c line 1034 should really be: |
| 85 | |
| 86 | if ( rl&& rl-iflags& RL_ROUTES_ADDED) |
| 87 | |
| 88 | and the corresponding code in add_routes (route.c, line 990) should |
| 89 | read: |
| 90 | |
| 91 | if ( rl&& !(rl-iflags& RL_ROUTES_ADDED)) |
| 92 | |
| 93 | ... enhancing my tests to add a "--route-nopull --route-ipv6 test"... |
| 94 | and indeed: |
| 95 | |
| 96 | ./t_client.sh: Zeile 200: 8787 Speicherzugriffsfehler ./openvpn $openvpn_conf $LOGDIR/$SUF:openvpn.log |
| 97 | |
| 98 | *bang* |
| 99 | |
| 100 | redirect_default_route_to_vpn (rl=0x0, rl6=0x80f801c, tt=0x8101f90, flags=0, |
| 101 | es=0x80dc860) at route.c:811 |
| 102 | 811 if (rl-flags& RG_ENABLE) |
| 103 | |
| 104 | a proper patch is attached... |
| 105 | |
| 106 | - ssl.c: it would be useful if andj or d12fk could review that - I'm not |
| 107 | actually sure I understand what changed, but it seems to be some |
| 108 | shuffling around of code and #ifdef ENABLE_CLIENT_CR, without actually |
| 109 | changing much. |
| 110 | |
| 111 | |
| 112 | the rest looks ok-ish to me... (but yes, I can understand that it took |
| 113 | you a heroic effort to merge that). |
| 114 | |
| 115 | }}} |