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

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

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

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