Hi, On Mon, Jan 2, 2023 at 5:51 PM Gert Doering <g...@greenie.muc.de> wrote:
> Hi, > > On Sat, Dec 31, 2022 at 05:40:49PM +0100, Gert Doering wrote: > > On Sat, Dec 17, 2022 at 07:09:34PM -0500, Selva Nair wrote: > > > tldr: Can we get CONNECTED,ERROR instead of CONNECTED,SUCCESS on route > > > errors? > > > > I think this makes sense. Not sure how complicated it is, and if we > > can make this before 2.6.0 ("some time in January"). > > So if I am reading this right, "CONNECTED,<something>" is produced > by management_set_state() -> log_entry_print(), with the "something" > being transported via "detail" -> "e->string". > > "CONNECTED" is only produced by > init.c::initialization_sequence_completed(), > and there already is a case for "CONNECTED,ERROR" though I don't understand > it... > > if (flags & ISC_ERRORS) > { > detail = "ERROR"; > } > > (which will also log "Initialization Sequence Completed *With Errors*", > but the rest of that branch might be a bit misleading, as it will > tell systemd "STATUS=Failed") > > > initialization_sequence_completed() is called from init.c::do_up() > (for client/p2p), forward.c, and mtcp.c/mudp.c (Server). > > The "--route-delay" case goes via forward.c::check_add_routes_action(), > which *can* signal ERROR. init.c::do_up() does not. > > Routes are (if no --route-delay) set up by do_init_tun(), which calls > do_route(). So adding an "&flags" parameter to that call chain which sets > ISC_ERROR if a route addition fails might be a viable approach. > I was also thinking along these lines. I think we can make add_route() return a status instead of void, and propagate it back to where it's needed in check_add_route_action(). Many of those functions that return void can return an int instead. All real worker functions that set a route : do_route_service, do_route_ipv6_servuce, do_route_ipapi, openvpn_execve_check, net_route_v4_add, net_route_v6_add etc. all return true or false or a status, so it's only their callers that need to pass it on. This is assuming "false" or error really means a serious error, not something trivial like a route already exists. And, there's the rub... I can't see how to get around this on all platforms. On Windows when using service or IPAPI we can check the error code for ERROR_OBJECT_ALREADY_EXISTS, on Linux using SITNL already ignores EEXIST, so those two cases are good to go. But what to do with openvpn_execve? I'm tempted to take a short-cut and implement this only for Windows and Linux when using SITNL, and leave others to continue reporting CONNECTED,SUCCESS on route errors. That would be easy to do as the only change required is: update netsh calls used for ipv6 routes on Windows to use IPAPI (that's an easy change worth doing anyway). As for those using route-method = exe on Windows, we may occasionally report CONNECTED,ERROR even for trivial "route already exists" cases. I think that is tolerable as no one should be using route-method=exe. The whole route-method option should have been deprecated long ago. Same with ip-win32 (just retain the manual option to please the demons). > ... and then I found ROUTE_BEFORE_TUN, ROUTE_AFTER_TUN... which adds > yet another call chain, which is totally irrelevant but would need that > extra argument as well... > And, --route-delay, I need to understand better what the code is doing > there... > I haven't looked into this either. Selva
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel