Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via email, send a message with subject or
body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH 03/15] inet: Refactor with getifaddrs() and add network route
getter function
(Jussi Laakkonen)
2. [PATCH] vpn: Do not do mixed declerations and code (Daniel Wagner)
3. [PATCH] src: Test return value of inet_pton consistently
(Daniel Wagner)
4. Re: [PATCH 00/15] Support SplitRouting on vpnd, drop vpnd route management
(Jussi Laakkonen)
----------------------------------------------------------------------
Date: Fri, 11 Dec 2020 16:06:46 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 03/15] inet: Refactor with getifaddrs() and add
network route getter function
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
On 12/11/20 2:45 PM, Daniel Wagner wrote:
> On Tue, Dec 08, 2020 at 12:17:49PM +0200, Jussi Laakkonen wrote:
>> +static int get_interface_addresses(struct interface_address *if_addr)
>> [...]
>> - memset(&ifr, 0, sizeof(ifr));
>> - ifr.ifr_ifindex = index;
>> + for (ifa = ifaddr; ifa; ifa = ifa->ifa_next) {
>> + if (!ifa->ifa_addr)
>> + continue;
>>
>> - if (ioctl(sk, SIOCGIFNAME, &ifr) < 0) {
>> - close(sk);
>> - return false;
>> + if (g_strcmp0(ifa->ifa_name, name) ||
>> + ifa->ifa_addr->sa_family !=
>> + if_addr->family)
>> + continue;
>
> If I understand this correctly (g_strcmp) is to make sure we are looking
> at the right interface. Do we really need to do this and why can't we
> use the index for it? Generally, we try to avoid the use of the
> interface name as lookup key. This avoids all sorts of problems
> with renaming the interface.
>
Yes, that was the point..
> I guess this snipped is just refactored from existing code. But that
> doesn't mean it was correct :)
>
and you are correct about the origins of it. If checking with indexes is
enough I'm more than fine with it. Did not want to change it too much I
guess.
> The rest looks not too scary. So let me review the rest first and if
> this is the only real comment from me we fix this in tree.
>
Awesome.
BR,
Jussi
------------------------------
Date: Fri, 11 Dec 2020 15:11:18 +0100
From: Daniel Wagner <[email protected]>
Subject: [PATCH] vpn: Do not do mixed declerations and code
To: [email protected]
Cc: Daniel Wagner <[email protected]>
Message-ID: <[email protected]>
gcc complains 'struct vpn_route' is declared within the code section.
By declaring it at the begin of the function we can also avoid the
additional helper variables.
---
plugins/vpn.c | 32 ++++++++++++++------------------
1 file changed, 14 insertions(+), 18 deletions(-)
diff --git a/plugins/vpn.c b/plugins/vpn.c
index 94d6e8591d88..3ed9c32268b4 100644
--- a/plugins/vpn.c
+++ b/plugins/vpn.c
@@ -1523,32 +1523,29 @@ static int save_route(GHashTable *routes, int family,
const char *network,
static int add_network_route(struct connection_data *data)
{
- char *network = NULL;
- char *gateway = NULL;
- char *netmask = NULL;
- int family;
+ struct vpn_route rt = { 0, };
int err;
if (!data)
return -EINVAL;
- family = connman_provider_get_family(data->provider);
- switch (family) {
+ rt.family = connman_provider_get_family(data->provider);
+ switch (rt.family) {
case PF_INET:
- err = connman_inet_get_route_addresses(data->index, &network,
- &netmask, &gateway);
+ err = connman_inet_get_route_addresses(data->index,
+ &rt.network, &rt.netmask, &rt.gateway);
break;
case PF_INET6:
err = connman_inet_ipv6_get_route_addresses(data->index,
- &network, &netmask, &gateway);
+ &rt.network, &rt.netmask, &rt.gateway);
break;
default:
- connman_error("invalid protocol family %d", family);
+ connman_error("invalid protocol family %d", rt.family);
return -EINVAL;
}
DBG("network %s gateway %s netmask %s for provider %p",
- network, gateway, netmask,
+ rt.network, rt.gateway,
rt.netmask,
data->provider);
if (err) {
@@ -1557,21 +1554,20 @@ static int add_network_route(struct connection_data
*data)
goto out;
}
- err = save_route(data->server_routes, family, network, netmask,
- gateway);
+ err = save_route(data->server_routes, rt.family, rt.network, rt.netmask,
+ rt.gateway);
if (err) {
connman_warn("failed to add network route for provider"
"%p", data->provider);
goto out;
}
- struct vpn_route route = { family, network, netmask, gateway };
- set_route(data, &route);
+ set_route(data, &rt);
out:
- g_free(network);
- g_free(gateway);
- g_free(netmask);
+ g_free(rt.network);
+ g_free(rt.netmask);
+ g_free(rt.gateway);
return 0;
}
--
2.29.2
------------------------------
Date: Fri, 11 Dec 2020 15:24:01 +0100
From: Daniel Wagner <[email protected]>
Subject: [PATCH] src: Test return value of inet_pton consistently
To: [email protected]
Cc: Daniel Wagner <[email protected]>
Message-ID: <[email protected]>
inet_pton return 1 on success and 0 or -1 on failure depending on the
error it wants to return. Use either '== 1' or '!= 1' consistently to
test if the call was succesfull. ConnMan doesn't destinguish between 0
and -1.
---
src/dhcpv6.c | 2 +-
src/dns-systemd-resolved.c | 4 ++--
src/inet.c | 22 +++++++++++-----------
src/ipaddress.c | 2 +-
src/iptables.c | 4 ++--
5 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/dhcpv6.c b/src/dhcpv6.c
index 2d5f8f6a601e..8b683599753f 100644
--- a/src/dhcpv6.c
+++ b/src/dhcpv6.c
@@ -945,7 +945,7 @@ static void do_dad(GDHCPClient *dhcp_client, struct
connman_dhcpv6 *dhcp)
ref_own_address(user_data);
- if (inet_pton(AF_INET6, address, &addr) < 0) {
+ if (inet_pton(AF_INET6, address, &addr) != 1) {
DBG("Invalid IPv6 address %s %d/%s", address,
-errno, strerror(errno));
goto fail;
diff --git a/src/dns-systemd-resolved.c b/src/dns-systemd-resolved.c
index 5fe306c3445d..912ab3fe8a42 100644
--- a/src/dns-systemd-resolved.c
+++ b/src/dns-systemd-resolved.c
@@ -106,7 +106,7 @@ static void setlinkdns_append(DBusMessageIter *iter, void
*user_data)
if (type == AF_INET) {
result = inet_pton(type, server, ipv4_bytes);
- if (!result) {
+ if (result != 1) {
DBG("Failed to parse IPv4 address: %s",
server);
return;
@@ -128,7 +128,7 @@ static void setlinkdns_append(DBusMessageIter *iter, void
*user_data)
&byte_array);
} else if (type == AF_INET6) {
result = inet_pton(type, server, ipv6_bytes);
- if (!result) {
+ if (result != 1) {
DBG("Failed to parse IPv6 address: %s", server);
return;
}
diff --git a/src/inet.c b/src/inet.c
index 2d30c3b6f6dc..df94d1eebaa7 100644
--- a/src/inet.c
+++ b/src/inet.c
@@ -121,11 +121,11 @@ int __connman_inet_modify_address(int cmd, int flags,
ifaddrmsg->ifa_index = index;
if (family == AF_INET) {
- if (inet_pton(AF_INET, address, &ipv4_addr) < 1)
+ if (inet_pton(AF_INET, address, &ipv4_addr) != 1)
return -1;
if (peer) {
- if (inet_pton(AF_INET, peer, &ipv4_dest) < 1)
+ if (inet_pton(AF_INET, peer, &ipv4_dest) != 1)
return -1;
err = __connman_inet_rtnl_addattr_l(header,
@@ -165,7 +165,7 @@ int __connman_inet_modify_address(int cmd, int flags,
return err;
}
} else if (family == AF_INET6) {
- if (inet_pton(AF_INET6, address, &ipv6_addr) < 1)
+ if (inet_pton(AF_INET6, address, &ipv6_addr) != 1)
return -1;
err = __connman_inet_rtnl_addattr_l(header,
@@ -707,7 +707,7 @@ int connman_inet_del_ipv6_network_route(int index, const
char *host,
rt.rtmsg_dst_len = prefix_len;
- if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) < 0) {
+ if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) != 1) {
err = -errno;
goto out;
}
@@ -757,7 +757,7 @@ int connman_inet_add_ipv6_network_route(int index, const
char *host,
rt.rtmsg_dst_len = prefix_len;
- if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) < 0) {
+ if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) != 1) {
err = -errno;
goto out;
}
@@ -773,7 +773,7 @@ int connman_inet_add_ipv6_network_route(int index, const
char *host,
*/
if (gateway && !__connman_inet_is_any_addr(gateway, AF_INET6) &&
- inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) > 0)
+ inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) == 1)
rt.rtmsg_flags |= RTF_GATEWAY;
rt.rtmsg_metric = 1;
@@ -816,7 +816,7 @@ int connman_inet_clear_ipv6_gateway_address(int index,
const char *gateway)
memset(&rt, 0, sizeof(rt));
- if (inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) < 0) {
+ if (inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) != 1) {
err = -errno;
goto out;
}
@@ -1255,7 +1255,7 @@ bool connman_inet_compare_subnet(int index, const char
*host)
if (!host)
return false;
- if (inet_pton(AF_INET, host, &haddr) <= 0)
+ if (inet_pton(AF_INET, host, &haddr) != 1)
return false;
if_addr.index = index;
@@ -1292,7 +1292,7 @@ bool connman_inet_compare_ipv6_subnet(int index, const
char *host)
struct in6_addr imask = { 0 };
struct in6_addr haddr = { 0 };
- if (inet_pton(AF_INET6, host, &haddr) <= 0)
+ if (inet_pton(AF_INET6, host, &haddr) != 1)
return false;
addr.index = index;
@@ -3179,7 +3179,7 @@ static int iproute_default_modify(int cmd, uint32_t
table_id, int ifindex,
ret = inet_pton(family, dst ? dst : gateway, buf);
g_free(dst);
- if (ret <= 0)
+ if (ret != 1)
return -EINVAL;
memset(&rth, 0, sizeof(rth));
@@ -3415,7 +3415,7 @@ static int get_nfs_server_ip(const char *cmdline_file,
const char *pnp_file,
addrstr[len] = '\0';
err = inet_pton(AF_INET, addrstr, addr);
- if (err <= 0) {
+ if (err != 1) {
connman_error("%s: Cannot convert to numeric addr \"%s\"\n",
__func__, addrstr);
err = -1;
diff --git a/src/ipaddress.c b/src/ipaddress.c
index f728eab319da..201d83454136 100644
--- a/src/ipaddress.c
+++ b/src/ipaddress.c
@@ -104,7 +104,7 @@ static bool check_ipv6_address(const char *address)
return false;
err = inet_pton(AF_INET6, address, buf);
- if (err > 0)
+ if (err == 1)
return true;
return false;
diff --git a/src/iptables.c b/src/iptables.c
index 6feb00747324..664b27f13d86 100644
--- a/src/iptables.c
+++ b/src/iptables.c
@@ -2931,7 +2931,7 @@ static int parse_ip_and_mask(const char *str, struct
in_addr *ip,
if (!tokens)
return -1;
- if (!inet_pton(AF_INET, tokens[0], ip)) {
+ if (inet_pton(AF_INET, tokens[0], ip) != 1) {
err = -1;
goto out;
}
@@ -2972,7 +2972,7 @@ static int parse_ipv6_and_mask(const char *str, struct
in6_addr *ip,
if (!tokens)
return -1;
- if (!inet_pton(AF_INET6, tokens[0], ip)) {
+ if (inet_pton(AF_INET6, tokens[0], ip) != 1) {
err = -1;
goto out;
}
--
2.29.2
------------------------------
Date: Fri, 11 Dec 2020 16:36:08 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 00/15] Support SplitRouting on vpnd, drop vpnd
route management
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
On 12/11/20 3:27 PM, Daniel Wagner wrote:
> Hi Jussi,
>
> 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
>> criteria.
>>
>> 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
>
> WireGuard gw
> local gw
> Splittet VPN
>
>
> 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.
>
> Thanks,
> Daniel
>
Great!
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
split routed.
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
https://git.sailfishos.org/mer-core/connman/blob/master/connman/src/storage.c
and systemd_login.c
https://git.sailfishos.org/mer-core/connman/blob/master/connman/src/systemd_login.c
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.
BR,
Jussi
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- [email protected]
To unsubscribe send an email to [email protected]
------------------------------
End of connman Digest, Vol 62, Issue 16
***************************************
