Sorry for a bit late reply, been busy.
On 12/11/20 5:14 PM, Daniel Wagner wrote:
On Fri, Dec 11, 2020 at 04:36:08PM +0200, Jussi Laakkonen wrote:
> 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.
That is really good to hear. I expected something like this.
> 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
There is this problem. If the bandages are somewhat contained and not
distributed over many files I wouldn't mind to get them. Do you have
link to it?
I think I have sometimes hard time in keeping up with these as some are
there for historic reasons, some are for our different devices (we have
developer mode over USB that tends to be working differently on
different devices because of different setups and there is different
types of internet sharing and...), and some are just simply on top of
our own changes based on our historic changes and... urgh, I think you
get the point.
We do have a plan to upstream/RFC our changes in our fork. I did an
overhaul of the differences some time ago, now it is just matter of
getting the different tasks into the working queue. I think we'll at
least attempt to push these bandages, fixes and improvements then. I
think the inet_pton() use was one of the items on the list and I guess
half of them was fixed quite a while ago.
And there are of course then those that have been fixed
differently/require time to solve the conflict.. All of them are in our
fork directly as commits.
> 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 split routed.
One thing we need to be clear to the user, that in such a scenario the
default traffic is not going over the VPN. This is more a UI thingy but
we should also have a big warning in the logs. I really want to avoid
the situation where the VPN service is at top of the service list and
it's not the default gateway.
Well that case I did not see at all. All I saw that when misconfigured
VPNC was put as split routed it did not have any routes set and network
route couldn't be enabled. In this case connmand then refused the change
and signaled connman-vpnd about the current split routing value (false),
which was then propagated to the client using the VPN D-Bus API.
Now to think of it, would it be reasonable to have the VPN D-Bus API
SetProperty() call wait until connmand replies to the call whether the
option could be changed or not or does this signaling approach be
enough, as it follows the way how the rest of the VPN values are changed
via VPN D-Bus API? I chose the current approach based on that, and the
simplicity, although multiple signaling in case of error but in that
case the error is caused by a misconfigured VPN..
> 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, storage.c
> and systemd_login.c
> can be checked there if there is interest and/or time.
BTW, I did try to cleanup the storage.c files beginning of the year. I
really hate the mess we have in there. The mix between provisioning and
configuration for both daemons ConnMan and VPN is just mind
boggling. How could so simple get so complex. I have a very rough
prototype which makes things way simpler. The main problem I run into
was how we support the different provisioning use case. You can either
provision VPN or ConnMan independently. This makes it really messy IMO
and it's an API issue. We cannot really cleanup this mess without
touching the D-Bus API.
Well, if you look that storage.c of ours it gets quite complex. We've
had a change to put all VPN related content to connman-vpn (so vpn_ and
provider_), and support for manual removal and adding of services and
having support for adding services over D-Bus as well there are some
other changes needed. Our storage.c also has that user change logic
embedded as of now. It was a design decision to avoid same content on
both connmand and vpnd side, hence both use the same. But unit testing
that whole process was a big challenge, however, almost all of it is now
simulated with unit tests.
I think we've had to amend the API in the old days. I'm not still aware
of all of them we have.
> 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.
Looking forward to it!