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
