On 12/11/20 3:27 PM, Daniel Wagner wrote:
On Tue, Dec 08, 2020 at 12:17:46PM +0200, Jussi Laakkonen wrote:
> SplitRouting is a VPN only setting that was managed by connmand and is also
> saved along the vpn_*/settings. This change exposes SplitRouting value also to
> vpnd, making it accessible over VPN D-Bus API as well. This allows to control
> the split routing value also on VPNs that are not connected yet. And being a
> VPN only value it seems natural to have it controllable over VPN D-Bus API.
> This does not affect the service moving, but extends it for internal use.
> In order to make this happen a lot of changes were required. First of all it
> is required to have synchronization of the value changes between connmand and
> vpnd. This is achieved by using service specific property changed signals sent
> always when the value changes, or is being set (e.g., loading the service), on
> both sides. This will result in vpnd informing about the change twice when the
> SplitRouting value is changed over VPN D-Bus API. This happens because VPN
> changes to connmand are propagated using property changed signals and after
> connmand has processed the SplitRouting signal connmand will notify the value
> again with a property changed signal to keep vpnd in sync in case of an error,
> for instance.
> Since service moving is utilized here for making the SplitRouting value changes
> on a connected VPN the route checks in plugins/vpn.c is enhanced to handle the
> routes better. If a VPN that is being set as a split routed does not have any
> other than the default route a network route will be attempted to be added. If
> that fails an error is printed and the resulting property changed signal will
> contain the current, not affected value of SplitRouting But if a VPN has a
> default route configured it will be prevented for SplitRouted VPNs. Also to
> make sure that a non-SplitRouted VPN will have a default route set it will be
> added if missing.
> And to make this happen inet.c was amended with new functionality to get route
> addresses. This resulted in some other changes to clean up the code and fix
> the issue of getifaddrs() not being able to interpret the destination address
> if the P-t-P connection has broadcast address also set. Combined many different
> getifaddrs() uses behind one function to have it a multi-purpose function for
> different uses. The added, inet.c internal get_interface_addresses() uses
> struct interface_address that allows AF specific addresses to be retrieved to
> their respective by copying the struct ifaddrs content that matches the given
> Also some minor change to service.c was required in order to propagate the
> SplitRouting value at all times. When loading a service it is imperative to
> apply the service settings after registering the D-Bus service because
> otherwise any signals cannot be sent about the loaded settings, if needed. This
> boils down to the fact that the internal gdbus implementation does not allow
> signals to be sent if the path given is not registered.
> Route management is also dropped from vpnd as a separate commit so it may be
> reverted easily if need arises. This was deemed to be unnecessary and confusing
> in discussions with Daniel Wagner on the ConnMan mailing list.
W00t, this is a lot of work! I looked through the series and all looks
reasonable sane. I spotted a few nitpicks, which I'll fix up directly
before applying it.
My not so trivial test setup is
working for IPv4. Now we just have to fix the IPv6 routing which is
completely broken. But this is something I try to address in my
'Set IPv6 default route' series.
We've had most of this running for a quite a long time and these newer
changes have been in for some time to get any bugs squeezed out, none so
far but if any rise I'll submit a patch. Some of the VPNs may be
apparently misconfigured that they do not report proper netmask even
(ifconfig verifies this). Not sure if it is something missing in our
1.32 based ConnMan which has a tonload of bandages on top and our own
changes or is it just a (misconfigured) test VPN (IPsec VPNC) server
which works in an odd manner.
I did ponder about the adding of the network route quite a long.
Especially in that netmask being inaddr_any, what should the network be
then as it cannot be resolved. I just thought having it as an error then
would be now the approach until that issue pops up and gets resolved,
only thing it affects is changing a connected misconfigured VPN to be
And about big work and other changes. At some point next year I guess it
will be time to send our multiuser changes as an RFC for people to test
and review, it includes having user specific WiFi and VPN (as we're
working in mobile environment, but there is work to be done in making
these configurable even) settings, ability to detect removed/added
services at runtime, user change listening over systemd logind to name
but a few. Plus simulated unit tests for user change operations in the
storage.c in which most of the changes are and separate unit tests
simulating systemd logind listening. These changes have been in use
since summer I think and we're fixing small issues here and there still.
But at some point I hope I can tweak it to patches, and hoping also to
get 1.38 upgrade ongoing next year as well. And these are no secrets,
can be checked there if there is interest and/or time.
There are also many other things that should be pushed to upstream for
review.. I guess many of them require some discussion at first. But I'll
get back at you when it is time for them.