3 p.m.
This set of patches addresses the issue of replying a VPN connection
request twice. Also the VPN connection requests are limited to one per
VPN. Connecting a previously failed VPN is also improved.
The plugins/vpn.c is enhanced to keep track of connection requests. If
there is a pending call to connect the VPN report -EALREADY and handle
that in provider.c. This must not create a state change indication but
only inform that the connection is still in progress.
Each connection request must be replied only once and vpn-provider.c:
do_connect() must not reply to the call immediately if -EINPROGRESS is
returned. This is because vpn-provider.c:connect_cb() will send an
appropriate reply later using the VPN plugin return code. If the VPN
is in failure state or is disconnecting a direct reply must be sent as
VPN plugin connect() is not called, thus connect_cb() is not reached.
Also, when a VPN settings are saved the previously set failure state
must be changed to idle. This then follows same approach as it is used
with error counters (reset when settings saved).
The error and state must be also reset, when user changes autoconnect
value for the VPN to true value. This is similar to when connecting a
VPN using D-Bus method Connect(). Without changing the state autoconnect
for the VPN (service.c:do_auto_connect()) does not work as with
CONNMAN_SERVICE_CONNET_REASON_AUTO a VPN in failure state should not
be connected.
Jussi Laakkonen (5):
vpn-provider: Change provider in failure state to idle when saving
vpn-provider: Always reply to connect request D-Bus message
vpn: Restrict connection requests to one per connection
provider: Handle -EALREADY in __connman_provider_connect()
service: Clear VPN error and state if AutoConnect is set on
plugins/vpn.c | 42 ++++++++++++++++++++++++++++++----
src/provider.c | 17 +++++++++-----
src/service.c | 56 +++++++++++++++++++++++++++++++++++++++-------
vpn/vpn-provider.c | 19 +++++++++++++++-
4 files changed, 116 insertions(+), 18 deletions(-)
--
2.20.1
3 p.m.
New subject: [PATCH 1/5] vpn-provider: Change provider in failure state to idle when saving
If the VPN provider is in failure state and it is saved the old state
must be cleared and changed to idle. This represents better the VPN
provider state in such scenario, the failure can be caused by connection
error or login error, both of which must be reset if the settings are
saved (most likely user changes settings to get the VPN to work).
---
vpn/vpn-provider.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 08792ecc..666bd016 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -879,6 +879,9 @@ static int vpn_provider_save(struct vpn_provider *provider)
reset_error_counters(provider);
+ if (provider->state == VPN_PROVIDER_STATE_FAILURE)
+ vpn_provider_set_state(provider, VPN_PROVIDER_STATE_IDLE);
+
if (provider->immutable) {
/*
* Do not save providers that are provisioned via .config
--
2.20.1
3 p.m.
New subject: [PATCH 2/5] vpn-provider: Reply to connect request only once
It is imperative to reply to the D-Bus method call only once. The
-EINPROGRESS returned by __vpn_provider_connect() should not be
immediately replied with an appropriate error. This is because the
connect_cb() will also send a reply based on the error returned by
the VPN plugin, or an empty one if there is no error.
In case the VPN is in failure state its state will be changed to idle
and -EINPROGRESS is reported as will be when VPN is being disconnected.
In both of these cases a reply to the method call is to be sent
directly.
Also, set VPN driver state to idle when a VPN is attempted to be
connected in failure state. This ensures that state is set for both
provider and the driver impl.
---
vpn/vpn-provider.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 666bd016..a6e2384d 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -503,7 +503,7 @@ static DBusMessage *do_connect(DBusConnection *conn, DBusMessage
*msg,
DBG("conn %p provider %p", conn, provider);
err = __vpn_provider_connect(provider, msg);
- if (err < 0)
+ if (err < 0 && err != -EINPROGRESS)
return __connman_error_failed(msg, -err);
return NULL;
@@ -1160,6 +1160,7 @@ static void connect_cb(struct vpn_provider *provider, void
*user_data,
int __vpn_provider_connect(struct vpn_provider *provider, DBusMessage *msg)
{
+ DBusMessage *reply;
int err;
DBG("provider %p state %d", provider, provider->state);
@@ -1172,6 +1173,10 @@ int __vpn_provider_connect(struct vpn_provider *provider,
DBusMessage *msg)
* attempt will continue as normal.
*/
case VPN_PROVIDER_STATE_FAILURE:
+ if (provider->driver && provider->driver->set_state)
+ provider->driver->set_state(provider,
+ VPN_PROVIDER_STATE_IDLE);
+
vpn_provider_set_state(provider, VPN_PROVIDER_STATE_IDLE);
/* fall through */
/*
@@ -1183,6 +1188,15 @@ int __vpn_provider_connect(struct vpn_provider *provider,
DBusMessage *msg)
* -EINPROGRESS to indicate that transition is in progress.
*/
case VPN_PROVIDER_STATE_DISCONNECT:
+ /*
+ * Failure transition or disconnecting does not yield a
+ * message to be sent. Send in progress message to avoid
+ * D-Bus LimitsExceeded error message.
+ */
+ reply = __connman_error_in_progress(msg);
+ if (reply)
+ g_dbus_send_message(connection, reply);
+
return -EINPROGRESS;
case VPN_PROVIDER_STATE_UNKNOWN:
case VPN_PROVIDER_STATE_IDLE:
--
2.20.1
3 p.m.
New subject: [PATCH 3/5] vpn: Restrict connection requests to one per connection
Utilize DBusPendingCall in struct connection_data to keep track of
connection requests for VPNs. There should not be more than one pending
call per VPN at a time. If there is a pending connection request return
-EALREADY to indicate the status.
---
plugins/vpn.c | 42 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/plugins/vpn.c b/plugins/vpn.c
index 6157dffb..5668c004 100644
--- a/plugins/vpn.c
+++ b/plugins/vpn.c
@@ -493,9 +493,18 @@ static void connect_reply(DBusPendingCall *call, void *user_data)
if (!dbus_pending_call_get_completed(call))
return;
+ if (call != data->call) {
+ connman_error("invalid call %p to VPN connect_reply data %p "
+ " call %p ", call, data, data->call);
+ dbus_pending_call_unref(call);
+ return;
+ }
+
DBG("user_data %p path %s", user_data, cb_data ? cb_data->path : NULL);
reply = dbus_pending_call_steal_reply(call);
+ if (!reply)
+ goto out;
dbus_error_init(&error);
@@ -534,7 +543,11 @@ static void connect_reply(DBusPendingCall *call, void *user_data)
dbus_message_unref(reply);
- dbus_pending_call_unref(call);
+out:
+ dbus_pending_call_unref(data->call);
+
+ data->call = NULL;
+ data->connect_pending = false;
}
static int connect_provider(struct connection_data *data, void *user_data,
@@ -553,7 +566,10 @@ static int connect_provider(struct connection_data *data, void
*user_data,
return -EINVAL;
}
- data->connect_pending = false;
+ if (data->connect_pending && data->call) {
+ connman_info("connect already pending");
+ return -EALREADY;
+ }
/* We need to pass original dbus sender to connman-vpnd,
* use a Connect2 method for that if the original dbus sender is set.
@@ -587,6 +603,14 @@ static int connect_provider(struct connection_data *data, void
*user_data,
return -EINVAL;
}
+ if (data->call) {
+ dbus_pending_call_cancel(data->call);
+ dbus_pending_call_unref(data->call);
+ }
+
+ data->call = call;
+ data->connect_pending = true;
+
if (cb_data) {
g_free(cb_data->path);
cb_data->path = g_strdup(data->path);
@@ -996,6 +1020,8 @@ static int disconnect_provider(struct connection_data *data)
static int provider_disconnect(struct connman_provider *provider)
{
+ int err = 0;
+
struct connection_data *data;
DBG("provider %p", provider);
@@ -1005,9 +1031,17 @@ static int provider_disconnect(struct connman_provider *provider)
return -EINVAL;
if (provider_is_connected(data))
- return disconnect_provider(data);
+ err = disconnect_provider(data);
- return 0;
+ if (data->call) {
+ dbus_pending_call_cancel(data->call);
+ dbus_pending_call_unref(data->call);
+ data->call = NULL;
+ }
+
+ data->connect_pending = false;
+
+ return err;
}
static void configuration_create_reply(DBusPendingCall *call, void *user_data)
--
2.20.1
3 p.m.
New subject: [PATCH 4/5] provider: Handle -EALREADY in __connman_provider_connect()
The error -EALREADY returned by vpn.c:provider_connect() in case there
is already a pending request for connecting the VPN is to be handled
separately from -EINPROGRESS. With -EALREADY the state has not changed
and should not be indicated. From service.c point of view the returned
status should still be -EINPROGRESS as the actual status of the VPN has
not changed.
---
src/provider.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/provider.c b/src/provider.c
index 8d90d5f8..7de96643 100644
--- a/src/provider.c
+++ b/src/provider.c
@@ -176,17 +176,24 @@ int __connman_provider_connect(struct connman_provider *provider,
else
return -EOPNOTSUPP;
- if (err < 0) {
- if (err != -EINPROGRESS)
- return err;
+ switch (err) {
+ case 0:
+ return 0;
+ case -EINPROGRESS:
provider_indicate_state(provider,
CONNMAN_SERVICE_STATE_ASSOCIATION);
-
+ /* fall through */
+ /*
+ * Return EINPROGRESS also for when there is an existing pending call.
+ * The state should not be indicated again but the real state is
+ * still in progress for the provider.
+ */
+ case -EALREADY:
return -EINPROGRESS;
}
- return 0;
+ return err;
}
int __connman_provider_remove_by_path(const char *path)
--
2.20.1
3 p.m.
New subject: [PATCH 5/5] service: Clear VPN error and state if AutoConnect is set on
Changing AutoConnect value to "on" must have the same effect as user
calling "Connect" method via D-Bus. The failure state is ignored in
do_auto_connect() if user called the VPN to connect.
This fixes the inconsistency with AutoConnect value setting, if user
sets AutoConnect to on clear error and set the service state to idle,
if in failure state. This makes it possible to clear the error and
getting VPN to connect immediately after toggling the AutoConnect on.
Implemented function to disable autoconnect from all but the selected
service. This is required with VPNs as only one VPN should be
automatically connected, hence each transport service can support one
VPN only.
---
src/service.c | 56 +++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 8 deletions(-)
diff --git a/src/service.c b/src/service.c
index 7e1446b7..4aa15ffc 100644
--- a/src/service.c
+++ b/src/service.c
@@ -3588,6 +3588,31 @@ void __connman_service_wispr_start(struct connman_service
*service,
__connman_wispr_start(service, type);
}
+static void disable_autoconnect_for_services(struct connman_service *exclude,
+ enum connman_service_type type)
+{
+ GList* list = NULL;
+
+ for (list = service_list; list; list = list->next) {
+ struct connman_service *service = list->data;
+
+ if (service->type != type)
+ continue;
+
+ if (service == exclude)
+ continue;
+
+ if (connman_service_set_autoconnect(service, false)) {
+ service_save(service);
+ connman_info("disabled autoconnect for %s",
+ service->name);
+ }
+ }
+}
+
+static void set_error(struct connman_service *service,
+ enum connman_service_error error);
+
static DBusMessage *set_property(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
@@ -3625,18 +3650,33 @@ static DBusMessage *set_property(DBusConnection *conn,
dbus_message_iter_get_basic(&value, &autoconnect);
- if (service->autoconnect == autoconnect)
- return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+ if (autoconnect && service->type == CONNMAN_SERVICE_TYPE_VPN) {
+ /*
+ * Changing the autoconnect flag on VPN to "on" should
+ * have the same effect as user connecting the VPN =
+ * clear previous error and change state to idle.
+ */
+ set_error(service, CONNMAN_SERVICE_ERROR_UNKNOWN);
- service->autoconnect = autoconnect;
+ if (service->state == CONNMAN_SERVICE_STATE_FAILURE) {
+ service->state = CONNMAN_SERVICE_STATE_IDLE;
+ state_changed(service);
+ }
- autoconnect_changed(service);
+ /* Disable autoconnect for all other VPN providers
+ * if autoconnect is set true. Only one VPN can have
+ * autoconnect enabled.
+ */
+ disable_autoconnect_for_services(service,
+ service->type);
+ }
- if (autoconnect)
- do_auto_connect(service,
+ if (connman_service_set_autoconnect(service, autoconnect)) {
+ service_save(service);
+ if (autoconnect)
+ do_auto_connect(service,
CONNMAN_SERVICE_CONNECT_REASON_AUTO);
-
- service_save(service);
+ }
} else if (g_str_equal(name, "Nameservers.Configuration")) {
DBusMessageIter entry;
GString *str;
--
2.20.1
10:30 a.m.
New subject: [PATCH 5/5] service: Clear VPN error and state if AutoConnect is
set on
Hi,
On Tue, Nov 12, 2019 at 04:00:55PM +0200, Jussi Laakkonen wrote:
Implemented function to disable autoconnect from all but the
selected
service. This is required with VPNs as only one VPN should be
automatically connected, hence each transport service can support one
VPN only.
This makes sense if only a full VPN tunnel is configured. But what
about the case you have two splittet tunnels? Is this a realistic
scenario?
Thanks,
Daniel
12:10 p.m.
New subject: [PATCH v2 5/5] service: Clear VPN error and state if AutoConnect is set on
Changing AutoConnect value to "true" must have the same effect as user
calling "Connect" method via D-Bus. The failure state is ignored in
do_auto_connect() if user called the VPN to connect.
This fixes the inconsistency with AutoConnect value setting, if user
sets AutoConnect to "true" clear error and set the service state to
idle, if VPN is in failure state. This makes it possible to get a
VPN connecting immediately after toggling the AutoConnect value.
---
Changes since v2:
* Removed function disable_autoconnect_for_services() and its use.
* Small changes to commit message.
src/service.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/src/service.c b/src/service.c
index 7e1446b7..cc87ed5c 100644
--- a/src/service.c
+++ b/src/service.c
@@ -3588,6 +3588,9 @@ void __connman_service_wispr_start(struct connman_service *service,
__connman_wispr_start(service, type);
}
+static void set_error(struct connman_service *service,
+ enum connman_service_error error);
+
static DBusMessage *set_property(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
@@ -3625,18 +3628,26 @@ static DBusMessage *set_property(DBusConnection *conn,
dbus_message_iter_get_basic(&value, &autoconnect);
- if (service->autoconnect == autoconnect)
- return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
-
- service->autoconnect = autoconnect;
+ if (autoconnect && service->type == CONNMAN_SERVICE_TYPE_VPN) {
+ /*
+ * Changing the autoconnect flag on VPN to "on" should
+ * have the same effect as user connecting the VPN =
+ * clear previous error and change state to idle.
+ */
+ set_error(service, CONNMAN_SERVICE_ERROR_UNKNOWN);
- autoconnect_changed(service);
+ if (service->state == CONNMAN_SERVICE_STATE_FAILURE) {
+ service->state = CONNMAN_SERVICE_STATE_IDLE;
+ state_changed(service);
+ }
+ }
- if (autoconnect)
- do_auto_connect(service,
+ if (connman_service_set_autoconnect(service, autoconnect)) {
+ service_save(service);
+ if (autoconnect)
+ do_auto_connect(service,
CONNMAN_SERVICE_CONNECT_REASON_AUTO);
-
- service_save(service);
+ }
} else if (g_str_equal(name, "Nameservers.Configuration")) {
DBusMessageIter entry;
GString *str;
--
2.20.1
5:26 p.m.
New subject: [PATCH v2 5/5] service: Clear VPN error and state if AutoConnect
is set on
Hi Jussi,
On Thu, Nov 14, 2019 at 01:10:00PM +0200, Jussi Laakkonen wrote:
Changing AutoConnect value to "true" must have the same
effect as user
calling "Connect" method via D-Bus. The failure state is ignored in
do_auto_connect() if user called the VPN to connect.
This fixes the inconsistency with AutoConnect value setting, if user
sets AutoConnect to "true" clear error and set the service state to
idle, if VPN is in failure state. This makes it possible to get a
VPN connecting immediately after toggling the AutoConnect value.
---
Changes since v2:
* Removed function disable_autoconnect_for_services() and its use.
* Small changes to commit message.
Thanks,
Daniel
10:29 a.m.
New subject: [PATCH 0/5] Track VPN connection requests and reply only once to
one
Hi Jussi,
On Tue, Nov 12, 2019 at 04:00:50PM +0200, Jussi Laakkonen wrote:
This set of patches addresses the issue of replying a VPN connection
request twice. Also the VPN connection requests are limited to one per
VPN. Connecting a previously failed VPN is also improved.
The plugins/vpn.c is enhanced to keep track of connection requests. If
there is a pending call to connect the VPN report -EALREADY and handle
that in provider.c. This must not create a state change indication but
only inform that the connection is still in progress.
Each connection request must be replied only once and vpn-provider.c:
do_connect() must not reply to the call immediately if -EINPROGRESS is
returned. This is because vpn-provider.c:connect_cb() will send an
appropriate reply later using the VPN plugin return code. If the VPN
is in failure state or is disconnecting a direct reply must be sent as
VPN plugin connect() is not called, thus connect_cb() is not reached.
Also, when a VPN settings are saved the previously set failure state
must be changed to idle. This then follows same approach as it is used
with error counters (reset when settings saved).
The error and state must be also reset, when user changes autoconnect
value for the VPN to true value. This is similar to when connecting a
VPN using D-Bus method Connect(). Without changing the state autoconnect
for the VPN (service.c:do_auto_connect()) does not work as with
CONNMAN_SERVICE_CONNET_REASON_AUTO a VPN in failure state should not
be connected.
I've applied patches 1-4. I reply on 5.
Thanks,
Daniel
10:42 a.m.
New subject: [PATCH 0/5] Track VPN connection requests and reply only once to
one
Hi Daniel,
I've applied patches 1-4. I reply on 5.
Thanks.
Waiting for the comment on 5. But the main idea behind 5th patch is that one could just
simply toggle the autoconnect value via D-Bus for a VPN to get it connecting without even
calling Connect() over D-Bus. And when that VPN is in failure state connection is not
attempted if only the autoconnect is toggled.
Furthermore, correct me if I'm wrong, but there can be only one VPN per physical
device in ConnMan, based on the physical -> vpn index relation in connection.c? That is
why autoconnect is with that 5th patch limited to one VPN connection only.
Cheers,
Jussi
11:15 a.m.
New subject: [PATCH 0/5] Track VPN connection requests and reply only once to
one
Hi Jussi,
On 14.11.19 10:42, Jussi Laakkonen wrote:
Waiting for the comment on 5.
Grrr, the mailing list did again not send you the response?
But the main idea behind 5th patch is that one could just simply
toggle the autoconnect value via D-Bus for a VPN to get it connecting without even calling
Connect() over D-Bus. And when that VPN is in failure state connection is not attempted if
only the autoconnect is toggled.
That is okay.
Furthermore, correct me if I'm wrong, but there can be only one
VPN per physical device in ConnMan, based on the physical -> vpn index relation in
connection.c? That is why autoconnect is with that 5th patch limited to one VPN connection
only.
My question was around this. So something which currently works (IIRC, I
played with it and I remember it worked) was setting up a full VPN
tunnel and having a splitted on top of it. My point is, with splitted
tunnels we could stack them and it makes sense at least for me :)
Thanks,
Daniel
11:29 a.m.
New subject: [PATCH 0/5] Track VPN connection requests and reply only once to
one
Grrr, the mailing list did again not send you the response?
Apparently it was delayed a long time as this UI did not show that
message until I posted a reply to the previous one. Now it shows this:
This makes sense if only a full VPN tunnel is configured. But what
about the case you have two splittet tunnels? Is this a realistic
scenario?
My question was around this. So something which currently works (IIRC, I
played with it and I remember it worked) was setting up a full VPN
tunnel and having a splitted on top of it. My point is, with splitted
tunnels we could stack them and it makes sense at least for me :)
Ah, we haven't used split tunnels at all. So I have no experience with them.
I think should look at them first about how they work and rethink it a bit. I'm
guessing we have gone into a bit different direction with the fork, as there
is a feature to enable/disable the VPN as default route. It may be that we've
implemented a feature that already existed?
I'll remove that disabling of autoconnect from the 5th patch and send a v2.
Sorry about that.
Cheers,
Jussi
917
days inactive
919
days old
13 comments
2 participants
participants (2)
-
Daniel Wagner -
Jussi Laakkonen