It is not feasible for every VPN plugin to check the errors in D-Bus reply sent by VPN agent in their own way. This kind of general use makes reacting to the canceled, timed out or no response replies sent by VPN agent more common and less error prone. The check function (vpn_agent_check_and_process_reply_error()) takes in the reply, callback to provider connect function and the pending D-Bus message, that will be utilized by the callback (vpn-provider.c:connect_cb()). This way proper notifications are passed forwards and appropriate reply is sent to the caller of the VPN plugin connection request (connmand). By sending the reply the caller (connmand) can then disable autoconnection from the VPN to avoid re-connection in case the user canceled the VPN agent dialog. The errors that are reacted to: - net.connman.vpn.Agent.Error.Canceled -> ECANCELED - org.freedesktop.DBus.Error.Timeout -> ETIMEDOUT - org.freedesktop.DBus.Error.NoReply -> ENOMSG - any other error from VPN agent is EACCES Error in VPN agent or canceling the VPN agent should not be an error in the VPN provider and, therefore, set the provider state to idle. Also, if task was already running stop it to avoid leaving VPN processes running. --- vpn/vpn-agent.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ vpn/vpn-agent.h | 4 ++++ 2 files changed, 52 insertions(+) diff --git a/vpn/vpn-agent.c b/vpn/vpn-agent.c index b0b582b7..82771b92 100644 --- a/vpn/vpn-agent.c +++ b/vpn/vpn-agent.c @@ -30,6 +30,8 @@ #include <gdbus.h> #include <connman/log.h> #include <connman/agent.h> +#include <connman/vpn-dbus.h> +#include <connman/task.h> #include <vpn/vpn-provider.h> #include "vpn-agent.h" @@ -143,3 +145,49 @@ void vpn_agent_append_user_info(DBusMessageIter *iter, request_input_append_user_info, &data); } + +int vpn_agent_check_and_process_reply_error(DBusMessage *reply, + struct vpn_provider *provider, + struct connman_task *task, + vpn_provider_connect_cb_t cb, void *user_data) +{ + DBusError error; + int err; + + if (!reply || !provider) + return EINVAL; + + dbus_error_init(&error); + + if (!dbus_set_error_from_message(&error, reply)) + return 0; + + DBG("D-Bus error: %s", error.name); + + if (!g_strcmp0(error.name, VPN_AGENT_INTERFACE ".Error.Canceled")) + err = ECANCELED; + else if (!g_strcmp0(error.name, "org.freedesktop.DBus.Error.Timeout")) + err = ETIMEDOUT; + else if (!g_strcmp0(error.name, "org.freedesktop.DBus.Error.NoReply")) + err = ENOMSG; + else + err = EACCES; + + dbus_error_free(&error); + + DBG("Error: %d/%s", err, strerror(err)); + + if (cb) + cb(provider, user_data, err); + + if (task) + connman_task_stop(task); + + /* + * VPN agent dialog cancel, timeout, broken connection should set the + * VPN back to idle state + */ + vpn_provider_set_state(provider, VPN_PROVIDER_STATE_IDLE); + + return err; +} diff --git a/vpn/vpn-agent.h b/vpn/vpn-agent.h index c7328d7f..be7f9dd9 100644 --- a/vpn/vpn-agent.h +++ b/vpn/vpn-agent.h @@ -38,6 +38,10 @@ bool vpn_agent_check_reply_has_dict(DBusMessage *reply); void vpn_agent_append_user_info(DBusMessageIter *iter, struct vpn_provider *provider, const char *username_str); +int vpn_agent_check_and_process_reply_error(DBusMessage *reply, + struct vpn_provider *provider, + struct connman_task *task, + vpn_provider_connect_cb_t cb, void *user_data); #ifdef __cplusplus } -- 2.20.1

Use vpn_agent_check_and_process_reply_error() to check and process VPN agent errors. Clear callback and pending D-Bus message (user_data) if error was processed to avoid calling the callback twice. --- Changes since V2: * If no reply received go to error. * Free private data if D-Bus error is received. vpn/plugins/openconnect.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/vpn/plugins/openconnect.c b/vpn/plugins/openconnect.c index 4117471c..b9125ede 100644 --- a/vpn/plugins/openconnect.c +++ b/vpn/plugins/openconnect.c @@ -311,12 +311,22 @@ static void request_input_cookie_reply(DBusMessage *reply, void *user_data) char *cookie = NULL, *servercert = NULL, *vpnhost = NULL; char *key; DBusMessageIter iter, dict; + int err_int; DBG("provider %p", data->provider); - if (!reply || dbus_message_get_type(reply) == DBUS_MESSAGE_TYPE_ERROR) + if (!reply) goto err; + err_int = vpn_agent_check_and_process_reply_error(reply, data->provider, + data->task, data->cb, data->user_data); + if (err_int) { + /* Ensure cb is called only once */ + data->cb = NULL; + data->user_data = NULL; + goto out; + } + if (!vpn_agent_check_reply_has_dict(reply)) goto err; @@ -387,6 +397,7 @@ err: vpn_provider_indicate_error(data->provider, VPN_PROVIDER_ERROR_AUTH_FAILED); +out: free_private_data(data); } -- 2.20.1

Use vpn_agent_check_and_process_reply_error() to check and process VPN agent errors. Clear callback and pending D-Bus message (user_data) if error was processed to avoid calling the callback twice. --- Changes since V2: * If no reply received stop processing. * Cleanup goto err. vpn/plugins/vpnc.c | 44 +++++++++++--------------------------------- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/vpn/plugins/vpnc.c b/vpn/plugins/vpnc.c index f222196a..9f33bb88 100644 --- a/vpn/plugins/vpnc.c +++ b/vpn/plugins/vpnc.c @@ -606,28 +606,20 @@ static void request_input_credentials_reply(DBusMessage *reply, void *user_data) char *secret = NULL, *username = NULL, *password = NULL; const char *key; DBusMessageIter iter, dict; - DBusError error; - int err_int = 0; + int err_int; DBG("provider %p", data->provider); - dbus_error_init(&error); - - if (dbus_set_error_from_message(&error, reply)) { - if (!g_strcmp0(error.name, VPN_AGENT_INTERFACE - ".Error.Canceled")) - err_int = ECONNABORTED; - - if (!g_strcmp0(error.name, VPN_AGENT_INTERFACE - ".Error.Timeout")) - err_int = ETIMEDOUT; - - dbus_error_free(&error); - - if (err_int) - goto abort; - + if (!reply) goto err; + + err_int = vpn_agent_check_and_process_reply_error(reply, data->provider, + data->task, data->cb, data->user_data); + if (err_int) { + /* Ensure cb is called only once */ + data->cb = NULL; + data->user_data = NULL; + return; } if (!vpn_agent_check_reply_has_dict(reply)) @@ -696,21 +688,7 @@ static void request_input_credentials_reply(DBusMessage *reply, void *user_data) return; err: - err_int = EACCES; - -abort: - vc_connect_done(data, err_int); - - switch (err_int) { - case EACCES: - vpn_provider_indicate_error(data->provider, - VPN_PROVIDER_ERROR_AUTH_FAILED); - break; - case ECONNABORTED: - case ETIMEDOUT: - vpn_provider_indicate_error(data->provider, - VPN_PROVIDER_ERROR_UNKNOWN); - } + vc_connect_done(data, EACCES); } static int request_input_credentials(struct vc_private_data *data, -- 2.20.1

Setter function for service autoconnect value. Calls autoconnect_changed() if change took place. Can be used to check if the change was made to, e.g., trigger saving of a service. --- include/service.h | 2 ++ src/service.c | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/include/service.h b/include/service.h index 97fdf7d5..4a129b49 100644 --- a/include/service.h +++ b/include/service.h @@ -131,6 +131,8 @@ const char *connman_service_get_proxy_url(struct connman_service *service); const char *connman_service_get_proxy_autoconfig(struct connman_service *service); bool connman_service_get_favorite(struct connman_service *service); bool connman_service_get_autoconnect(struct connman_service *service); +bool connman_service_set_autoconnect(struct connman_service *service, + bool autoconnect); /* Return non-zero value to terminate the loop, zero to continue */ typedef int (* connman_service_iterate_cb) (struct connman_service *service, diff --git a/src/service.c b/src/service.c index 0c5e647e..f71a3de7 100644 --- a/src/service.c +++ b/src/service.c @@ -1705,6 +1705,18 @@ static void autoconnect_changed(struct connman_service *service) DBUS_TYPE_BOOLEAN, &autoconnect); } +bool connman_service_set_autoconnect(struct connman_service *service, + bool autoconnect) +{ + if (service->autoconnect == autoconnect) + return false; + + service->autoconnect = autoconnect; + autoconnect_changed(service); + + return true; +} + static void append_security(DBusMessageIter *iter, void *user_data) { struct connman_service *service = user_data; -- 2.20.1

Add connman_provider_set_autoconnect() for setting the VPN autoconnect value. If a change took place save the VPN service. It is imperative to save the VPN service when autoconnect changes since otherwise the change is done only to run-time service settings and not written to disk if user has canceled VPN agent dialog, for example. Otherwise the data within connman and service file is out of sync, and next restart of connman will be a race between each VPNs that were attempted to connect but canceled by user. --- include/provider.h | 2 ++ src/provider.c | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/include/provider.h b/include/provider.h index d28651ad..aa563edf 100644 --- a/include/provider.h +++ b/include/provider.h @@ -111,6 +111,8 @@ int connman_provider_set_domain(struct connman_provider *provider, const char *domain); int connman_provider_set_nameservers(struct connman_provider *provider, char * const *nameservers); +void connman_provider_set_autoconnect(struct connman_provider *provider, + bool flag); int connman_provider_append_route(struct connman_provider *provider, const char *key, const char *value); diff --git a/src/provider.c b/src/provider.c index 9d9741e1..33671b2f 100644 --- a/src/provider.c +++ b/src/provider.c @@ -579,6 +579,17 @@ int connman_provider_set_nameservers(struct connman_provider *provider, return 0; } +void connman_provider_set_autoconnect(struct connman_provider *provider, + bool flag) +{ + if (provider && provider->vpn_service) { + /* Save VPN service if autoconnect value changes */ + if (connman_service_set_autoconnect(provider->vpn_service, + flag)) + __connman_service_save(provider->vpn_service); + } +} + static void unregister_provider(gpointer data) { struct connman_provider *provider = data; -- 2.20.1