[PATCH 0/2] Implement connmand state monitoring to vpnd

[PATCH 0/2] Implement connmand state monitoring to vpnd

[Jussi Laakkonen]

Implement connmand state monitoring into vpnd to keep track of the state
changes in regards to connectivity and D-Bus registration. vpnd retrieves the
state from connmand using Manager interface when started and otherwise it
monitors PropertyChanged signals as well as utilizes D-Bus service monitoring
to get the changes.

This change prevents connecting VPNs when connmand is not yet online or has
gone away from D-Bus (e.g., restart). In case VPN is connected before the state
has been queried a delayed connection function is utilized to connect that VPN
when online/ready state is reached.

In case connmand is offline or gone from D-Bus ENOLINK is returned as an error.
This is handled in plugins/vpn.c similar to EINPROGRESS reply as it is not a
real VPN error, EINPROGRESS is replied in case the connection is being delayed.

Jussi Laakkonen (2):
  vpn-provider: Implement connmand online state checking
  vpn: Handle ENOLINK error in connect callback

 plugins/vpn.c      |  10 +-
 vpn/vpn-provider.c | 361 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 369 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Add a complete mechanism to track connmand state and query it if
necessary to avoid connecting VPNs when there is either no connmand or
no network to use. This also makes VPNs to disconnect when connmand
loses its online state or disappears.

The connmand state listener uses net.connman.Manager interface to get
the state using GetProperties at startup. PropertyChanged signal is
monitored for state changes to update the state. State is changed and
queried when the D-Bus service listener is notified. Connmand state is
tracked within vpnd with a boolean: "true" = online/ready, "false" =
offline/idle.

Also a feature to support delayed connecting of VPNs is added. It may
happen in a situation where ConnMan status is not queried yet and
a request to connect is received over D-Bus. In this case a timeout
function is added to the main loop that runs with 1s interval. When the
delayed connect function is running it keeps on trying until connmand
state is online/ready. If connection request comes when the state is
queried and it is not online/ready ENOLINK is returned as an error
("NoCarrier" D-Bus msg).
---
 vpn/vpn-provider.c | 361 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 360 insertions(+), 1 deletion(-)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 59c805c5..8c86168e 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -3,7 +3,7 @@
  *  ConnMan VPN daemon
  *
  *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
- *  Copyright (C) 2019  Jolla Ltd. All rights reserved.
+ *  Copyright (C) 2019-2021  Jolla Ltd. All rights reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -24,6 +24,13 @@
 #include <config.h>
 #endif
 
+#define STATE_INTERVAL_DEFAULT		0
+
+#define CONNMAN_STATE_ONLINE 		"online"
+#define CONNMAN_STATE_OFFLINE 		"offline"
+#define CONNMAN_STATE_READY 		"ready"
+#define CONNMAN_STATE_IDLE 		"idle"
+
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
@@ -89,15 +96,55 @@ struct vpn_provider {
 	struct connman_ipaddress *prev_ipv4_addr;
 	struct connman_ipaddress *prev_ipv6_addr;
 	void *plugin_data;
+	unsigned int do_connect_timeout;
 	unsigned int auth_error_counter;
 	unsigned int conn_error_counter;
 	unsigned int signal_watch;
 };
 
+struct vpn_provider_connect_data {
+	DBusConnection *conn;
+	DBusMessage *msg;
+	struct vpn_provider *provider;
+};
+
+static unsigned int get_connman_state_timeout = 0;
+
+static guint connman_signal_watch;
+static guint connman_service_watch;
+
+static bool connman_online = false;
+static bool state_query_completed = false;
+
 static void append_properties(DBusMessageIter *iter,
 				struct vpn_provider *provider);
 static int vpn_provider_save(struct vpn_provider *provider);
 
+static void get_connman_state();
+
+static void set_state(const char* new_state)
+{
+	if (!new_state || !*new_state)
+		return;
+
+	DBG("old state %s new state %s",
+		connman_online ?
+		CONNMAN_STATE_ONLINE "/" CONNMAN_STATE_READY :
+		CONNMAN_STATE_OFFLINE "/" CONNMAN_STATE_IDLE,
+		new_state);
+
+	/* States "online" and "ready" mean connman is online */
+	if (g_ascii_strcasecmp(new_state, CONNMAN_STATE_ONLINE) == 0 ||
+		g_ascii_strcasecmp(new_state, CONNMAN_STATE_READY) == 0)
+		connman_online = true;
+	/* Everything else means connman is offline */
+	else
+		connman_online = false;
+
+	DBG("set state %s connman_online=%s ",
+		new_state, connman_online ? "true" : "false");
+}
+
 static void free_route(gpointer data)
 {
 	struct vpn_route *route = data;
@@ -740,6 +787,65 @@ static DBusMessage *clear_property(DBusConnection *conn, DBusMessage *msg,
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
+static gboolean do_connect_timeout_function(gpointer data)
+{
+	DBG("");
+
+	/*
+	 * Keep in main loop if no agents are present yet or if connman is not
+	 * yet online
+	 */
+	if (!connman_agent_get_info(NULL, NULL, NULL) || !connman_online) {
+		return G_SOURCE_CONTINUE;
+	} else {
+		struct vpn_provider_connect_data *cdata = data;
+		struct vpn_provider *provider = cdata->provider;
+		int err;
+
+		provider->do_connect_timeout = 0;
+		err = __vpn_provider_connect(provider, cdata->msg);
+
+		if (err < 0 && err != -EINPROGRESS) {
+			g_dbus_send_message(cdata->conn,
+				__connman_error_failed(cdata->msg, -err));
+			cdata->msg = NULL;
+		}
+
+		return G_SOURCE_REMOVE;
+	}
+}
+
+static void do_connect_timeout_free(gpointer data)
+{
+	struct vpn_provider_connect_data *cdata = data;
+
+	if (cdata->msg)
+		g_dbus_send_message(cdata->conn,
+			__connman_error_operation_aborted(cdata->msg));
+
+	dbus_connection_unref(cdata->conn);
+	g_free(data);
+}
+
+static void do_connect_later(struct vpn_provider *provider,
+					DBusConnection *conn, DBusMessage *msg)
+{
+	struct vpn_provider_connect_data *cdata =
+		g_try_new0(struct vpn_provider_connect_data, 1);
+
+	cdata->conn = dbus_connection_ref(conn);
+	cdata->msg = dbus_message_ref(msg);
+	cdata->provider = provider;
+
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
+	provider->do_connect_timeout =
+		g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
+					do_connect_timeout_function, cdata,
+					do_connect_timeout_free);
+}
+
 static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 								void *data)
 {
@@ -748,6 +854,27 @@ static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 
 	DBG("conn %p provider %p", conn, provider);
 
+	/* Check if any agents have been added, otherwise delay connecting */
+	if (!connman_agent_get_info(NULL, NULL, NULL)) {
+		DBG("Provider %s start delayed because no VPN agent is present",
+			provider->identifier);
+		do_connect_later(provider, conn, msg);
+		return NULL;
+	}
+
+	if (!connman_online) {
+		if (state_query_completed) {
+			DBG("Provider %s not started because connman "
+				"not online/ready", provider->identifier);
+			return __connman_error_failed(msg, ENOLINK);
+		} else {
+			DBG("Provider %s start delayed because connman "
+				"state not queried", provider->identifier);
+			do_connect_later(provider, conn, msg);
+			return NULL;
+		}
+	}
+
 	err = __vpn_provider_connect(provider, msg);
 	if (err < 0 && err != -EINPROGRESS)
 		return __connman_error_failed(msg, -err);
@@ -1269,6 +1396,9 @@ static void provider_destruct(struct vpn_provider *provider)
 {
 	DBG("provider %p", provider);
 
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
 	if (provider->notify_id != 0)
 		g_source_remove(provider->notify_id);
 
@@ -3214,6 +3344,213 @@ static void remove_unprovisioned_providers(void)
 	g_strfreev(providers);
 }
 
+static gboolean connman_property_changed(DBusConnection *conn,
+				DBusMessage *message,
+				void *user_data)
+{
+	DBusMessageIter iter, value;
+	const char *key;
+	const char *signature = DBUS_TYPE_STRING_AS_STRING
+				DBUS_TYPE_VARIANT_AS_STRING;
+
+	if (!dbus_message_has_signature(message, signature)) {
+		connman_error("vpn connman property signature \"%s\" "
+			"does not match expected \"%s\"",
+			dbus_message_get_signature(message), signature);
+		return TRUE;
+	}
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+
+	dbus_message_iter_next(&iter);
+	dbus_message_iter_recurse(&iter, &value);
+
+	DBG("key %s", key);
+
+	if (g_str_equal(key, "State")) {
+		const char *str;
+
+		if (dbus_message_iter_get_arg_type(&value) != DBUS_TYPE_STRING)
+			return TRUE;
+
+		dbus_message_iter_get_basic(&value, &str);
+		set_state(str);
+	}
+
+	return TRUE;
+}
+
+static void get_connman_state_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply = NULL;
+	DBusError error;
+	DBusMessageIter iter, array, dict, value;
+
+	const char *signature =DBUS_TYPE_ARRAY_AS_STRING
+		DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+		DBUS_TYPE_STRING_AS_STRING
+		DBUS_TYPE_VARIANT_AS_STRING
+		DBUS_DICT_ENTRY_END_CHAR_AS_STRING;
+
+	const char *key;
+	const char *str;
+
+	DBG("");
+
+	if (!dbus_pending_call_get_completed(call))
+		goto done;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&error);
+
+	if (dbus_set_error_from_message(&error, reply)) {
+		connman_error("%s", error.message);
+
+		/* In case of timeout re-add the state function to main
+		 * event loop */
+		if (g_ascii_strcasecmp(error.name, DBUS_ERROR_TIMEOUT) == 0) {
+			DBG("D-Bus timeout, re-add get_connman_state()");
+			get_connman_state();
+		} else {
+			dbus_error_free(&error);
+			goto done;
+		}
+	}
+
+	if (!dbus_message_has_signature(reply, signature)) {
+		connman_error("vpnd signature \"%s\" does not match "
+							"expected \"%s\"",
+			dbus_message_get_signature(reply), signature);
+		goto done;
+	}
+
+	if (!dbus_message_iter_init(reply, &array))
+		goto done;
+
+	dbus_message_iter_recurse(&array, &dict);
+
+	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
+		dbus_message_iter_recurse(&dict, &iter);
+
+		dbus_message_iter_get_basic(&iter, &key);
+
+		dbus_message_iter_next(&iter);
+		dbus_message_iter_recurse(&iter, &value);
+
+		if (g_ascii_strcasecmp(key, "State") == 0 &&
+			dbus_message_iter_get_arg_type(&value) ==
+				DBUS_TYPE_STRING) {
+			dbus_message_iter_get_basic(&value, &str);
+
+			DBG("Got initial state %s", str);
+
+			set_state(str);
+
+			/* No need to process further */
+			break;
+		}
+
+		dbus_message_iter_next(&dict);
+	}
+
+	state_query_completed = true;
+
+done:
+	if (reply)
+		dbus_message_unref(reply);
+
+	if (call)
+		dbus_pending_call_unref(call);
+}
+
+static gboolean run_get_connman_state(gpointer user_data)
+{
+	const char *path = "/";
+	const char *method = "GetProperties";
+	gboolean rval = FALSE;
+
+	DBusMessage *msg = NULL;
+	DBusPendingCall *call = NULL;
+
+	DBG("");
+
+	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
+		CONNMAN_MANAGER_INTERFACE, method);
+
+	if (!msg)
+		goto out;
+
+	if (!(rval = g_dbus_send_message_with_reply(connection, msg,
+		&call, -1))) {
+		connman_error("Cannot call %s on %s",
+			method, CONNMAN_MANAGER_INTERFACE);
+		goto out;
+	}
+
+	if (!call) {
+		connman_error("set pending call failed");
+		goto error;
+	}
+
+	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
+		NULL, NULL)) {
+		connman_error("set notify to pending call failed");
+		goto error;
+	}
+
+out:
+	if (msg)
+		dbus_message_unref(msg);
+
+	/* In case sending was success, unset timeout function id */
+	if (rval) {
+		DBG("unsetting get_connman_state_timeout id");
+		get_connman_state_timeout = 0;
+	}
+
+	/* Return FALSE in case of success to remove from main event loop */
+	return !rval;
+
+error:
+	if (call)
+		dbus_pending_call_unref(call);
+
+	rval = G_SOURCE_REMOVE;
+	goto out;
+}
+
+static void get_connman_state()
+{
+	if (get_connman_state_timeout)
+		return;
+
+	get_connman_state_timeout =
+		g_timeout_add(STATE_INTERVAL_DEFAULT,
+			run_get_connman_state, NULL);
+}
+
+static void connman_service_watch_connected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+	get_connman_state();
+}
+
+static void connman_service_watch_disconnected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+
+	set_state(CONNMAN_STATE_IDLE);
+
+	/* Set state query variable to initial state */
+	state_query_completed = false;
+}
+
 int __vpn_provider_init(void)
 {
 	int err;
@@ -3233,6 +3570,20 @@ int __vpn_provider_init(void)
 
 	provider_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
 						NULL, unregister_provider);
+
+	connman_service_watch = g_dbus_add_service_watch(connection,
+					CONNMAN_SERVICE,
+					connman_service_watch_connected,
+					connman_service_watch_disconnected,
+					NULL, NULL);
+
+	connman_signal_watch = g_dbus_add_signal_watch(connection,
+					CONNMAN_SERVICE, NULL,
+					CONNMAN_MANAGER_INTERFACE,
+					PROPERTY_CHANGED,
+					connman_property_changed,
+					NULL, NULL);
+
 	return 0;
 }
 
@@ -3247,5 +3598,13 @@ void __vpn_provider_cleanup(void)
 	g_hash_table_destroy(provider_hash);
 	provider_hash = NULL;
 
+	if (get_connman_state_timeout) {
+		if (!g_source_remove(get_connman_state_timeout))
+			connman_error("connman state timeout not removed");
+	}
+
+	g_dbus_remove_watch(connection, connman_service_watch);
+	g_dbus_remove_watch(connection, connman_signal_watch);
+
 	dbus_connection_unref(connection);
 }
-- 
2.20.1

Re: [PATCH 1/2] vpn-provider: Implement connmand online state checking

[Daniel Wagner]

Hi Jussi,

Just a bunch of small nits.

On Tue, Aug 17, 2021 at 06:14:42PM +0300, Jussi Laakkonen wrote:
> +static gboolean do_connect_timeout_function(gpointer data)
> +{
> +	DBG("");
> +
> +	/*
> +	 * Keep in main loop if no agents are present yet or if connman is not
> +	 * yet online
> +	 */
> +	if (!connman_agent_get_info(NULL, NULL, NULL) || !connman_online) {
> +		return G_SOURCE_CONTINUE;

Doesn't this get the mainloop busy looping?

> +	} else {

As there is a return above, no need to do a else statement. Makes also
the indention one less :)

> +		struct vpn_provider_connect_data *cdata = data;
> +		struct vpn_provider *provider = cdata->provider;
> +		int err;
> +
> +		provider->do_connect_timeout = 0;
> +		err = __vpn_provider_connect(provider, cdata->msg);
> +
> +		if (err < 0 && err != -EINPROGRESS) {
> +			g_dbus_send_message(cdata->conn,
> +				__connman_error_failed(cdata->msg, -err));
> +			cdata->msg = NULL;
> +		}
> +
> +		return G_SOURCE_REMOVE;
> +	}
> +}

[...]

> +static void do_connect_later(struct vpn_provider *provider,
> +					DBusConnection *conn, DBusMessage *msg)
> +{
> +	struct vpn_provider_connect_data *cdata =
> +		g_try_new0(struct vpn_provider_connect_data, 1);

Do a g_new0() instead of a g_try_new0().

> +
> +	cdata->conn = dbus_connection_ref(conn);
> +	cdata->msg = dbus_message_ref(msg);
> +	cdata->provider = provider;
> +
> +	if (provider->do_connect_timeout)
> +		g_source_remove(provider->do_connect_timeout);
> +
> +	provider->do_connect_timeout =
> +		g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
> +					do_connect_timeout_function, cdata,
> +					do_connect_timeout_free);
> +}
> +

[...]

> +static gboolean run_get_connman_state(gpointer user_data)
> +{
> +	const char *path = "/";
> +	const char *method = "GetProperties";
> +	gboolean rval = FALSE;
> +
> +	DBusMessage *msg = NULL;
> +	DBusPendingCall *call = NULL;
> +
> +	DBG("");
> +
> +	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
> +		CONNMAN_MANAGER_INTERFACE, method);
> +
> +	if (!msg)
> +		goto out;
> +
> +	if (!(rval = g_dbus_send_message_with_reply(connection, msg,
> +		&call, -1))) {

Move the 'rval = ...' outside the if condition.

> +		connman_error("Cannot call %s on %s",
> +			method, CONNMAN_MANAGER_INTERFACE);
> +		goto out;
> +	}
> +
> +	if (!call) {
> +		connman_error("set pending call failed");
> +		goto error;
> +	}
> +
> +	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
> +		NULL, NULL)) {
> +		connman_error("set notify to pending call failed");
> +		goto error;
> +	}
> +
> +out:
> +	if (msg)
> +		dbus_message_unref(msg);
> +
> +	/* In case sending was success, unset timeout function id */
> +	if (rval) {
> +		DBG("unsetting get_connman_state_timeout id");
> +		get_connman_state_timeout = 0;
> +	}
> +
> +	/* Return FALSE in case of success to remove from main event loop */
> +	return !rval;
> +
> +error:
> +	if (call)
> +		dbus_pending_call_unref(call);
> +
> +	rval = G_SOURCE_REMOVE;

This looks wrong. Shouldn't this be either TRUE or FALSE?

> +	goto out;

Please try to avoid jumping up again. Forward goto's are easy to read
but the backwards one tend to be hard to grasp (at least for me).

Daniel

Re: [PATCH 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Hi Daniel,

Thanks for the comments. This piece of code has been running in our fork 
for approx. 3 years but that does not mean there wouldn't be any issues 
to fix or to improve.

On 8/17/21 7:55 PM, Daniel Wagner wrote:
> Hi Jussi,
> 
> Just a bunch of small nits.
> 
> On Tue, Aug 17, 2021 at 06:14:42PM +0300, Jussi Laakkonen wrote:
>> +static gboolean do_connect_timeout_function(gpointer data)
>> +{
>> +	DBG("");
>> +
>> +	/*
>> +	 * Keep in main loop if no agents are present yet or if connman is not
>> +	 * yet online
>> +	 */
>> +	if (!connman_agent_get_info(NULL, NULL, NULL) || !connman_online) {
>> +		return G_SOURCE_CONTINUE;
> 
> Doesn't this get the mainloop busy looping?
> 

As far as I can remember this is intentional. This is added to mainloop 
with 1s delay in do_connect_later() for the delayed VPN connect so I 
guess looping with 1s interval is not too busy?

>> +	} else {
> 
> As there is a return above, no need to do a else statement. Makes also
> the indention one less :)

Yep, that cleans up the rest as well a bit.

> 
>> +		struct vpn_provider_connect_data *cdata = data;
>> +		struct vpn_provider *provider = cdata->provider;
>> +		int err;
>> +
>> +		provider->do_connect_timeout = 0;
>> +		err = __vpn_provider_connect(provider, cdata->msg);
>> +
>> +		if (err < 0 && err != -EINPROGRESS) {
>> +			g_dbus_send_message(cdata->conn,
>> +				__connman_error_failed(cdata->msg, -err));
>> +			cdata->msg = NULL;
>> +		}
>> +
>> +		return G_SOURCE_REMOVE;
>> +	}
>> +}
> 
> [...]
> 
>> +static void do_connect_later(struct vpn_provider *provider,
>> +					DBusConnection *conn, DBusMessage *msg)
>> +{
>> +	struct vpn_provider_connect_data *cdata =
>> +		g_try_new0(struct vpn_provider_connect_data, 1);
> 
> Do a g_new0() instead of a g_try_new0().

Right, will do this was following the old coding style :).

> 
>> +
>> +	cdata->conn = dbus_connection_ref(conn);
>> +	cdata->msg = dbus_message_ref(msg);
>> +	cdata->provider = provider;
>> +
>> +	if (provider->do_connect_timeout)
>> +		g_source_remove(provider->do_connect_timeout);
>> +
>> +	provider->do_connect_timeout =
>> +		g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
>> +					do_connect_timeout_function, cdata,
>> +					do_connect_timeout_free);
>> +}
>> +
> 
> [...]
> 
>> +static gboolean run_get_connman_state(gpointer user_data)
>> +{
>> +	const char *path = "/";
>> +	const char *method = "GetProperties";
>> +	gboolean rval = FALSE;
>> +
>> +	DBusMessage *msg = NULL;
>> +	DBusPendingCall *call = NULL;
>> +
>> +	DBG("");
>> +
>> +	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
>> +		CONNMAN_MANAGER_INTERFACE, method);
>> +
>> +	if (!msg)
>> +		goto out;
>> +
>> +	if (!(rval = g_dbus_send_message_with_reply(connection, msg,
>> +		&call, -1))) {
> 
> Move the 'rval = ...' outside the if condition.

Thanks for pointing that out. It looks indeed messy.

> 
>> +		connman_error("Cannot call %s on %s",
>> +			method, CONNMAN_MANAGER_INTERFACE);
>> +		goto out;
>> +	}
>> +
>> +	if (!call) {
>> +		connman_error("set pending call failed");
>> +		goto error;
>> +	}
>> +
>> +	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
>> +		NULL, NULL)) {
>> +		connman_error("set notify to pending call failed");
>> +		goto error;
>> +	}
>> +
>> +out:
>> +	if (msg)
>> +		dbus_message_unref(msg);
>> +
>> +	/* In case sending was success, unset timeout function id */
>> +	if (rval) {
>> +		DBG("unsetting get_connman_state_timeout id");
>> +		get_connman_state_timeout = 0;
>> +	}
>> +
>> +	/* Return FALSE in case of success to remove from main event loop */
>> +	return !rval;
>> +
>> +error:
>> +	if (call)
>> +		dbus_pending_call_unref(call);
>> +
>> +	rval = G_SOURCE_REMOVE;
> 
> This looks wrong. Shouldn't this be either TRUE or FALSE?
> 

Hm, as a return from a GSourceFunc G_SOURCE_{REMOVE,CONTINUE} is 
documented to be a more memorable name. But I think as well that in this 
case use of those as mixed, in addition to the comment, just creates 
more confusion. Yeah, maybe it is better to use here that as clear 
TRUE/FALSE.

>> +	goto out;
> 
> Please try to avoid jumping up again. Forward goto's are easy to read
> but the backwards one tend to be hard to grasp (at least for me).
> 

Ouch, that indeed looks a bit messy, hah. I'll refactor that a lot. 
There seems to be indentation refactoring needed as well.

I'll send v2 today at some point.

Cheers,
  Jussi

[PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Add a complete mechanism to track connmand state and query it if
necessary to avoid connecting VPNs when there is either no connmand or
no network to use. This also makes VPNs to disconnect when connmand
loses its online state or disappears.

The connmand state listener uses net.connman.Manager interface to get
the state using GetProperties at startup. PropertyChanged signal is
monitored for state changes to update the state. State is changed and
queried when the D-Bus service listener is notified. Connmand state is
tracked within vpnd with a boolean: "true" = online/ready, "false" =
offline/idle.

Also a feature to support delayed connecting of VPNs is added. It may
happen in a situation where ConnMan status is not queried yet and
a request to connect is received over D-Bus. In this case a timeout
function is added to the main loop that runs with 1s interval. When the
delayed connect function is running it keeps on trying until connmand
state is online/ready. If connection request comes when the state is
queried and it is not online/ready ENOLINK is returned as an error
("NoCarrier" D-Bus msg).
---
Changes since v2:
 * Cleanups and fixes to follow coding style.
 * Refactor run_get_connman_state().
 * Replace g_try_new0() with g_new().

 vpn/vpn-provider.c | 362 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 361 insertions(+), 1 deletion(-)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 59c805c5..72245794 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -3,7 +3,7 @@
  *  ConnMan VPN daemon
  *
  *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
- *  Copyright (C) 2019  Jolla Ltd. All rights reserved.
+ *  Copyright (C) 2019-2021  Jolla Ltd. All rights reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -24,6 +24,13 @@
 #include <config.h>
 #endif
 
+#define STATE_INTERVAL_DEFAULT		0
+
+#define CONNMAN_STATE_ONLINE 		"online"
+#define CONNMAN_STATE_OFFLINE 		"offline"
+#define CONNMAN_STATE_READY 		"ready"
+#define CONNMAN_STATE_IDLE 		"idle"
+
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
@@ -89,15 +96,55 @@ struct vpn_provider {
 	struct connman_ipaddress *prev_ipv4_addr;
 	struct connman_ipaddress *prev_ipv6_addr;
 	void *plugin_data;
+	unsigned int do_connect_timeout;
 	unsigned int auth_error_counter;
 	unsigned int conn_error_counter;
 	unsigned int signal_watch;
 };
 
+struct vpn_provider_connect_data {
+	DBusConnection *conn;
+	DBusMessage *msg;
+	struct vpn_provider *provider;
+};
+
+static unsigned int get_connman_state_timeout = 0;
+
+static guint connman_signal_watch;
+static guint connman_service_watch;
+
+static bool connman_online = false;
+static bool state_query_completed = false;
+
 static void append_properties(DBusMessageIter *iter,
 				struct vpn_provider *provider);
 static int vpn_provider_save(struct vpn_provider *provider);
 
+static void get_connman_state();
+
+static void set_state(const char* new_state)
+{
+	if (!new_state || !*new_state)
+		return;
+
+	DBG("old state %s new state %s",
+				connman_online ?
+				CONNMAN_STATE_ONLINE "/" CONNMAN_STATE_READY :
+				CONNMAN_STATE_OFFLINE "/" CONNMAN_STATE_IDLE,
+				new_state);
+
+	/* States "online" and "ready" mean connman is online */
+	if (!g_ascii_strcasecmp(new_state, CONNMAN_STATE_ONLINE) ||
+			!g_ascii_strcasecmp(new_state, CONNMAN_STATE_READY))
+		connman_online = true;
+	/* Everything else means connman is offline */
+	else
+		connman_online = false;
+
+	DBG("set state %s connman_online=%s ", new_state,
+				connman_online ? "true" : "false");
+}
+
 static void free_route(gpointer data)
 {
 	struct vpn_route *route = data;
@@ -740,6 +787,64 @@ static DBusMessage *clear_property(DBusConnection *conn, DBusMessage *msg,
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
+static gboolean do_connect_timeout_function(gpointer data)
+{
+	struct vpn_provider_connect_data *cdata = data;
+	struct vpn_provider *provider = cdata->provider;
+	int err;
+
+	DBG("");
+
+	/*
+	 * Keep in main loop if no agents are present yet or if connman is not
+	 * yet online.
+	 */
+	if (!connman_agent_get_info(NULL, NULL, NULL) || !connman_online)
+		return G_SOURCE_CONTINUE;
+
+	provider->do_connect_timeout = 0;
+	err = __vpn_provider_connect(provider, cdata->msg);
+
+	if (err < 0 && err != -EINPROGRESS) {
+		g_dbus_send_message(cdata->conn,
+				__connman_error_failed(cdata->msg, -err));
+		cdata->msg = NULL;
+	}
+
+	return G_SOURCE_REMOVE;
+}
+
+static void do_connect_timeout_free(gpointer data)
+{
+	struct vpn_provider_connect_data *cdata = data;
+
+	if (cdata->msg)
+		g_dbus_send_message(cdata->conn,
+				__connman_error_operation_aborted(cdata->msg));
+
+	dbus_connection_unref(cdata->conn);
+	g_free(data);
+}
+
+static void do_connect_later(struct vpn_provider *provider,
+					DBusConnection *conn, DBusMessage *msg)
+{
+	struct vpn_provider_connect_data *cdata =
+				g_new0(struct vpn_provider_connect_data, 1);
+
+	cdata->conn = dbus_connection_ref(conn);
+	cdata->msg = dbus_message_ref(msg);
+	cdata->provider = provider;
+
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
+	provider->do_connect_timeout =
+			g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
+					do_connect_timeout_function, cdata,
+					do_connect_timeout_free);
+}
+
 static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 								void *data)
 {
@@ -748,6 +853,29 @@ static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 
 	DBG("conn %p provider %p", conn, provider);
 
+	/* Check if any agents have been added, otherwise delay connecting */
+	if (!connman_agent_get_info(NULL, NULL, NULL)) {
+		DBG("Provider %s start delayed because no VPN agent is present",
+							provider->identifier);
+		do_connect_later(provider, conn, msg);
+		return NULL;
+	}
+
+	if (!connman_online) {
+		if (state_query_completed) {
+			DBG("Provider %s not started because connman "
+						"not online/ready",
+						provider->identifier);
+			return __connman_error_failed(msg, ENOLINK);
+		} else {
+			DBG("Provider %s start delayed because connman "
+						"state not queried",
+						provider->identifier);
+			do_connect_later(provider, conn, msg);
+			return NULL;
+		}
+	}
+
 	err = __vpn_provider_connect(provider, msg);
 	if (err < 0 && err != -EINPROGRESS)
 		return __connman_error_failed(msg, -err);
@@ -1269,6 +1397,9 @@ static void provider_destruct(struct vpn_provider *provider)
 {
 	DBG("provider %p", provider);
 
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
 	if (provider->notify_id != 0)
 		g_source_remove(provider->notify_id);
 
@@ -3214,6 +3345,213 @@ static void remove_unprovisioned_providers(void)
 	g_strfreev(providers);
 }
 
+static gboolean connman_property_changed(DBusConnection *conn,
+				DBusMessage *message,
+				void *user_data)
+{
+	DBusMessageIter iter, value;
+	const char *key;
+	const char *signature = DBUS_TYPE_STRING_AS_STRING
+				DBUS_TYPE_VARIANT_AS_STRING;
+
+	if (!dbus_message_has_signature(message, signature)) {
+		connman_error("vpn connman property signature \"%s\" "
+					"does not match expected \"%s\"",
+					dbus_message_get_signature(message),
+					signature);
+		return TRUE;
+	}
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+
+	dbus_message_iter_next(&iter);
+	dbus_message_iter_recurse(&iter, &value);
+
+	DBG("key %s", key);
+
+	if (g_str_equal(key, "State")) {
+		const char *str;
+
+		if (dbus_message_iter_get_arg_type(&value) != DBUS_TYPE_STRING)
+			return TRUE;
+
+		dbus_message_iter_get_basic(&value, &str);
+		set_state(str);
+	}
+
+	return TRUE;
+}
+
+static void get_connman_state_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply = NULL;
+	DBusError error;
+	DBusMessageIter iter, array, dict, value;
+
+	const char *signature = DBUS_TYPE_ARRAY_AS_STRING
+				DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+				DBUS_TYPE_STRING_AS_STRING
+				DBUS_TYPE_VARIANT_AS_STRING
+				DBUS_DICT_ENTRY_END_CHAR_AS_STRING;
+
+	const char *key;
+	const char *str;
+
+	DBG("");
+
+	if (!dbus_pending_call_get_completed(call))
+		goto done;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&error);
+
+	if (dbus_set_error_from_message(&error, reply)) {
+		connman_error("%s", error.message);
+
+		/*
+		 * In case of timeout re-add the state function to main
+		 * event loop.
+		 */
+		if (g_ascii_strcasecmp(error.name, DBUS_ERROR_TIMEOUT) == 0) {
+			DBG("D-Bus timeout, re-add get_connman_state()");
+			get_connman_state();
+		} else {
+			dbus_error_free(&error);
+			goto done;
+		}
+	}
+
+	if (!dbus_message_has_signature(reply, signature)) {
+		connman_error("vpnd signature \"%s\" does not match "
+					"expected \"%s\"",
+					dbus_message_get_signature(reply),
+					signature);
+		goto done;
+	}
+
+	if (!dbus_message_iter_init(reply, &array))
+		goto done;
+
+	dbus_message_iter_recurse(&array, &dict);
+
+	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
+		dbus_message_iter_recurse(&dict, &iter);
+
+		dbus_message_iter_get_basic(&iter, &key);
+
+		dbus_message_iter_next(&iter);
+		dbus_message_iter_recurse(&iter, &value);
+
+		if (g_ascii_strcasecmp(key, "State") == 0 &&
+				dbus_message_iter_get_arg_type(&value) ==
+						DBUS_TYPE_STRING) {
+			dbus_message_iter_get_basic(&value, &str);
+
+			DBG("Got initial state %s", str);
+
+			set_state(str);
+
+			/* No need to process further */
+			break;
+		}
+
+		dbus_message_iter_next(&dict);
+	}
+
+	state_query_completed = true;
+
+done:
+	if (reply)
+		dbus_message_unref(reply);
+
+	if (call)
+		dbus_pending_call_unref(call);
+}
+
+static gboolean run_get_connman_state(gpointer user_data)
+{
+	const char *path = "/";
+	const char *method = "GetProperties";
+	gboolean rval = FALSE;
+
+	DBusMessage *msg = NULL;
+	DBusPendingCall *call = NULL;
+
+	DBG("");
+
+	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
+					CONNMAN_MANAGER_INTERFACE, method);
+	if (!msg)
+		goto out;
+
+	rval = g_dbus_send_message_with_reply(connection, msg, &call, -1);
+	if (!rval) {
+		connman_error("Cannot call %s on %s", method,
+						CONNMAN_MANAGER_INTERFACE);
+		goto out;
+	}
+
+	if (!call) {
+		connman_error("set pending call failed");
+		rval = FALSE;
+		goto out;
+	}
+
+	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
+								NULL, NULL)) {
+		connman_error("set notify to pending call failed");
+
+		if (call)
+			dbus_pending_call_unref(call);
+
+		rval = FALSE;
+	}
+
+out:
+	if (msg)
+		dbus_message_unref(msg);
+
+	/* In case sending was success, unset timeout function id */
+	if (rval) {
+		DBG("unsetting get_connman_state_timeout id");
+		get_connman_state_timeout = 0;
+	}
+
+	/* Return FALSE in case of success to remove from main event loop */
+	return !rval;
+}
+
+static void get_connman_state()
+{
+	if (get_connman_state_timeout)
+		return;
+
+	get_connman_state_timeout = g_timeout_add(STATE_INTERVAL_DEFAULT,
+						run_get_connman_state, NULL);
+}
+
+static void connman_service_watch_connected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+	get_connman_state();
+}
+
+static void connman_service_watch_disconnected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+
+	set_state(CONNMAN_STATE_IDLE);
+
+	/* Set state query variable to initial state */
+	state_query_completed = false;
+}
+
 int __vpn_provider_init(void)
 {
 	int err;
@@ -3233,6 +3571,20 @@ int __vpn_provider_init(void)
 
 	provider_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
 						NULL, unregister_provider);
+
+	connman_service_watch = g_dbus_add_service_watch(connection,
+					CONNMAN_SERVICE,
+					connman_service_watch_connected,
+					connman_service_watch_disconnected,
+					NULL, NULL);
+
+	connman_signal_watch = g_dbus_add_signal_watch(connection,
+					CONNMAN_SERVICE, NULL,
+					CONNMAN_MANAGER_INTERFACE,
+					PROPERTY_CHANGED,
+					connman_property_changed,
+					NULL, NULL);
+
 	return 0;
 }
 
@@ -3247,5 +3599,13 @@ void __vpn_provider_cleanup(void)
 	g_hash_table_destroy(provider_hash);
 	provider_hash = NULL;
 
+	if (get_connman_state_timeout) {
+		if (!g_source_remove(get_connman_state_timeout))
+			connman_error("connman state timeout not removed");
+	}
+
+	g_dbus_remove_watch(connection, connman_service_watch);
+	g_dbus_remove_watch(connection, connman_signal_watch);
+
 	dbus_connection_unref(connection);
 }
-- 
2.20.1

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Hi Daniel,

Sorry, I spotted some error with this as apparently our fork has 
something again done differently somewhere. I'll send another version 
when I find the cause. Does not seem to function correctly on top of 1.40.

Sorry for the inconvenience.

- Jussi

On 8/18/21 1:31 PM, Jussi Laakkonen wrote:
> Add a complete mechanism to track connmand state and query it if
> necessary to avoid connecting VPNs when there is either no connmand or
> no network to use. This also makes VPNs to disconnect when connmand
> loses its online state or disappears.
> 
> The connmand state listener uses net.connman.Manager interface to get
> the state using GetProperties at startup. PropertyChanged signal is
> monitored for state changes to update the state. State is changed and
> queried when the D-Bus service listener is notified. Connmand state is
> tracked within vpnd with a boolean: "true" = online/ready, "false" =
> offline/idle.
> 
> Also a feature to support delayed connecting of VPNs is added. It may
> happen in a situation where ConnMan status is not queried yet and
> a request to connect is received over D-Bus. In this case a timeout
> function is added to the main loop that runs with 1s interval. When the
> delayed connect function is running it keeps on trying until connmand
> state is online/ready. If connection request comes when the state is
> queried and it is not online/ready ENOLINK is returned as an error
> ("NoCarrier" D-Bus msg).
> ---
> Changes since v2:
>   * Cleanups and fixes to follow coding style.
>   * Refactor run_get_connman_state().
>   * Replace g_try_new0() with g_new().
> 
>   vpn/vpn-provider.c | 362 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 361 insertions(+), 1 deletion(-)
> 
> diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
> index 59c805c5..72245794 100644
> --- a/vpn/vpn-provider.c
> +++ b/vpn/vpn-provider.c
> @@ -3,7 +3,7 @@
>    *  ConnMan VPN daemon
>    *
>    *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
> - *  Copyright (C) 2019  Jolla Ltd. All rights reserved.
> + *  Copyright (C) 2019-2021  Jolla Ltd. All rights reserved.
>    *
>    *  This program is free software; you can redistribute it and/or modify
>    *  it under the terms of the GNU General Public License version 2 as
> @@ -24,6 +24,13 @@
>   #include <config.h>
>   #endif
>   
> +#define STATE_INTERVAL_DEFAULT		0
> +
> +#define CONNMAN_STATE_ONLINE 		"online"
> +#define CONNMAN_STATE_OFFLINE 		"offline"
> +#define CONNMAN_STATE_READY 		"ready"
> +#define CONNMAN_STATE_IDLE 		"idle"
> +
>   #include <errno.h>
>   #include <stdio.h>
>   #include <string.h>
> @@ -89,15 +96,55 @@ struct vpn_provider {
>   	struct connman_ipaddress *prev_ipv4_addr;
>   	struct connman_ipaddress *prev_ipv6_addr;
>   	void *plugin_data;
> +	unsigned int do_connect_timeout;
>   	unsigned int auth_error_counter;
>   	unsigned int conn_error_counter;
>   	unsigned int signal_watch;
>   };
>   
> +struct vpn_provider_connect_data {
> +	DBusConnection *conn;
> +	DBusMessage *msg;
> +	struct vpn_provider *provider;
> +};
> +
> +static unsigned int get_connman_state_timeout = 0;
> +
> +static guint connman_signal_watch;
> +static guint connman_service_watch;
> +
> +static bool connman_online = false;
> +static bool state_query_completed = false;
> +
>   static void append_properties(DBusMessageIter *iter,
>   				struct vpn_provider *provider);
>   static int vpn_provider_save(struct vpn_provider *provider);
>   
> +static void get_connman_state();
> +
> +static void set_state(const char* new_state)
> +{
> +	if (!new_state || !*new_state)
> +		return;
> +
> +	DBG("old state %s new state %s",
> +				connman_online ?
> +				CONNMAN_STATE_ONLINE "/" CONNMAN_STATE_READY :
> +				CONNMAN_STATE_OFFLINE "/" CONNMAN_STATE_IDLE,
> +				new_state);
> +
> +	/* States "online" and "ready" mean connman is online */
> +	if (!g_ascii_strcasecmp(new_state, CONNMAN_STATE_ONLINE) ||
> +			!g_ascii_strcasecmp(new_state, CONNMAN_STATE_READY))
> +		connman_online = true;
> +	/* Everything else means connman is offline */
> +	else
> +		connman_online = false;
> +
> +	DBG("set state %s connman_online=%s ", new_state,
> +				connman_online ? "true" : "false");
> +}
> +
>   static void free_route(gpointer data)
>   {
>   	struct vpn_route *route = data;
> @@ -740,6 +787,64 @@ static DBusMessage *clear_property(DBusConnection *conn, DBusMessage *msg,
>   	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
>   }
>   
> +static gboolean do_connect_timeout_function(gpointer data)
> +{
> +	struct vpn_provider_connect_data *cdata = data;
> +	struct vpn_provider *provider = cdata->provider;
> +	int err;
> +
> +	DBG("");
> +
> +	/*
> +	 * Keep in main loop if no agents are present yet or if connman is not
> +	 * yet online.
> +	 */
> +	if (!connman_agent_get_info(NULL, NULL, NULL) || !connman_online)
> +		return G_SOURCE_CONTINUE;
> +
> +	provider->do_connect_timeout = 0;
> +	err = __vpn_provider_connect(provider, cdata->msg);
> +
> +	if (err < 0 && err != -EINPROGRESS) {
> +		g_dbus_send_message(cdata->conn,
> +				__connman_error_failed(cdata->msg, -err));
> +		cdata->msg = NULL;
> +	}
> +
> +	return G_SOURCE_REMOVE;
> +}
> +
> +static void do_connect_timeout_free(gpointer data)
> +{
> +	struct vpn_provider_connect_data *cdata = data;
> +
> +	if (cdata->msg)
> +		g_dbus_send_message(cdata->conn,
> +				__connman_error_operation_aborted(cdata->msg));
> +
> +	dbus_connection_unref(cdata->conn);
> +	g_free(data);
> +}
> +
> +static void do_connect_later(struct vpn_provider *provider,
> +					DBusConnection *conn, DBusMessage *msg)
> +{
> +	struct vpn_provider_connect_data *cdata =
> +				g_new0(struct vpn_provider_connect_data, 1);
> +
> +	cdata->conn = dbus_connection_ref(conn);
> +	cdata->msg = dbus_message_ref(msg);
> +	cdata->provider = provider;
> +
> +	if (provider->do_connect_timeout)
> +		g_source_remove(provider->do_connect_timeout);
> +
> +	provider->do_connect_timeout =
> +			g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
> +					do_connect_timeout_function, cdata,
> +					do_connect_timeout_free);
> +}
> +
>   static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
>   								void *data)
>   {
> @@ -748,6 +853,29 @@ static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
>   
>   	DBG("conn %p provider %p", conn, provider);
>   
> +	/* Check if any agents have been added, otherwise delay connecting */
> +	if (!connman_agent_get_info(NULL, NULL, NULL)) {
> +		DBG("Provider %s start delayed because no VPN agent is present",
> +							provider->identifier);
> +		do_connect_later(provider, conn, msg);
> +		return NULL;
> +	}
> +
> +	if (!connman_online) {
> +		if (state_query_completed) {
> +			DBG("Provider %s not started because connman "
> +						"not online/ready",
> +						provider->identifier);
> +			return __connman_error_failed(msg, ENOLINK);
> +		} else {
> +			DBG("Provider %s start delayed because connman "
> +						"state not queried",
> +						provider->identifier);
> +			do_connect_later(provider, conn, msg);
> +			return NULL;
> +		}
> +	}
> +
>   	err = __vpn_provider_connect(provider, msg);
>   	if (err < 0 && err != -EINPROGRESS)
>   		return __connman_error_failed(msg, -err);
> @@ -1269,6 +1397,9 @@ static void provider_destruct(struct vpn_provider *provider)
>   {
>   	DBG("provider %p", provider);
>   
> +	if (provider->do_connect_timeout)
> +		g_source_remove(provider->do_connect_timeout);
> +
>   	if (provider->notify_id != 0)
>   		g_source_remove(provider->notify_id);
>   
> @@ -3214,6 +3345,213 @@ static void remove_unprovisioned_providers(void)
>   	g_strfreev(providers);
>   }
>   
> +static gboolean connman_property_changed(DBusConnection *conn,
> +				DBusMessage *message,
> +				void *user_data)
> +{
> +	DBusMessageIter iter, value;
> +	const char *key;
> +	const char *signature = DBUS_TYPE_STRING_AS_STRING
> +				DBUS_TYPE_VARIANT_AS_STRING;
> +
> +	if (!dbus_message_has_signature(message, signature)) {
> +		connman_error("vpn connman property signature \"%s\" "
> +					"does not match expected \"%s\"",
> +					dbus_message_get_signature(message),
> +					signature);
> +		return TRUE;
> +	}
> +
> +	if (!dbus_message_iter_init(message, &iter))
> +		return TRUE;
> +
> +	dbus_message_iter_get_basic(&iter, &key);
> +
> +	dbus_message_iter_next(&iter);
> +	dbus_message_iter_recurse(&iter, &value);
> +
> +	DBG("key %s", key);
> +
> +	if (g_str_equal(key, "State")) {
> +		const char *str;
> +
> +		if (dbus_message_iter_get_arg_type(&value) != DBUS_TYPE_STRING)
> +			return TRUE;
> +
> +		dbus_message_iter_get_basic(&value, &str);
> +		set_state(str);
> +	}
> +
> +	return TRUE;
> +}
> +
> +static void get_connman_state_reply(DBusPendingCall *call, void *user_data)
> +{
> +	DBusMessage *reply = NULL;
> +	DBusError error;
> +	DBusMessageIter iter, array, dict, value;
> +
> +	const char *signature = DBUS_TYPE_ARRAY_AS_STRING
> +				DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
> +				DBUS_TYPE_STRING_AS_STRING
> +				DBUS_TYPE_VARIANT_AS_STRING
> +				DBUS_DICT_ENTRY_END_CHAR_AS_STRING;
> +
> +	const char *key;
> +	const char *str;
> +
> +	DBG("");
> +
> +	if (!dbus_pending_call_get_completed(call))
> +		goto done;
> +
> +	reply = dbus_pending_call_steal_reply(call);
> +
> +	dbus_error_init(&error);
> +
> +	if (dbus_set_error_from_message(&error, reply)) {
> +		connman_error("%s", error.message);
> +
> +		/*
> +		 * In case of timeout re-add the state function to main
> +		 * event loop.
> +		 */
> +		if (g_ascii_strcasecmp(error.name, DBUS_ERROR_TIMEOUT) == 0) {
> +			DBG("D-Bus timeout, re-add get_connman_state()");
> +			get_connman_state();
> +		} else {
> +			dbus_error_free(&error);
> +			goto done;
> +		}
> +	}
> +
> +	if (!dbus_message_has_signature(reply, signature)) {
> +		connman_error("vpnd signature \"%s\" does not match "
> +					"expected \"%s\"",
> +					dbus_message_get_signature(reply),
> +					signature);
> +		goto done;
> +	}
> +
> +	if (!dbus_message_iter_init(reply, &array))
> +		goto done;
> +
> +	dbus_message_iter_recurse(&array, &dict);
> +
> +	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> +		dbus_message_iter_recurse(&dict, &iter);
> +
> +		dbus_message_iter_get_basic(&iter, &key);
> +
> +		dbus_message_iter_next(&iter);
> +		dbus_message_iter_recurse(&iter, &value);
> +
> +		if (g_ascii_strcasecmp(key, "State") == 0 &&
> +				dbus_message_iter_get_arg_type(&value) ==
> +						DBUS_TYPE_STRING) {
> +			dbus_message_iter_get_basic(&value, &str);
> +
> +			DBG("Got initial state %s", str);
> +
> +			set_state(str);
> +
> +			/* No need to process further */
> +			break;
> +		}
> +
> +		dbus_message_iter_next(&dict);
> +	}
> +
> +	state_query_completed = true;
> +
> +done:
> +	if (reply)
> +		dbus_message_unref(reply);
> +
> +	if (call)
> +		dbus_pending_call_unref(call);
> +}
> +
> +static gboolean run_get_connman_state(gpointer user_data)
> +{
> +	const char *path = "/";
> +	const char *method = "GetProperties";
> +	gboolean rval = FALSE;
> +
> +	DBusMessage *msg = NULL;
> +	DBusPendingCall *call = NULL;
> +
> +	DBG("");
> +
> +	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
> +					CONNMAN_MANAGER_INTERFACE, method);
> +	if (!msg)
> +		goto out;
> +
> +	rval = g_dbus_send_message_with_reply(connection, msg, &call, -1);
> +	if (!rval) {
> +		connman_error("Cannot call %s on %s", method,
> +						CONNMAN_MANAGER_INTERFACE);
> +		goto out;
> +	}
> +
> +	if (!call) {
> +		connman_error("set pending call failed");
> +		rval = FALSE;
> +		goto out;
> +	}
> +
> +	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
> +								NULL, NULL)) {
> +		connman_error("set notify to pending call failed");
> +
> +		if (call)
> +			dbus_pending_call_unref(call);
> +
> +		rval = FALSE;
> +	}
> +
> +out:
> +	if (msg)
> +		dbus_message_unref(msg);
> +
> +	/* In case sending was success, unset timeout function id */
> +	if (rval) {
> +		DBG("unsetting get_connman_state_timeout id");
> +		get_connman_state_timeout = 0;
> +	}
> +
> +	/* Return FALSE in case of success to remove from main event loop */
> +	return !rval;
> +}
> +
> +static void get_connman_state()
> +{
> +	if (get_connman_state_timeout)
> +		return;
> +
> +	get_connman_state_timeout = g_timeout_add(STATE_INTERVAL_DEFAULT,
> +						run_get_connman_state, NULL);
> +}
> +
> +static void connman_service_watch_connected(DBusConnection *conn,
> +				void *user_data)
> +{
> +	DBG("");
> +	get_connman_state();
> +}
> +
> +static void connman_service_watch_disconnected(DBusConnection *conn,
> +				void *user_data)
> +{
> +	DBG("");
> +
> +	set_state(CONNMAN_STATE_IDLE);
> +
> +	/* Set state query variable to initial state */
> +	state_query_completed = false;
> +}
> +
>   int __vpn_provider_init(void)
>   {
>   	int err;
> @@ -3233,6 +3571,20 @@ int __vpn_provider_init(void)
>   
>   	provider_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
>   						NULL, unregister_provider);
> +
> +	connman_service_watch = g_dbus_add_service_watch(connection,
> +					CONNMAN_SERVICE,
> +					connman_service_watch_connected,
> +					connman_service_watch_disconnected,
> +					NULL, NULL);
> +
> +	connman_signal_watch = g_dbus_add_signal_watch(connection,
> +					CONNMAN_SERVICE, NULL,
> +					CONNMAN_MANAGER_INTERFACE,
> +					PROPERTY_CHANGED,
> +					connman_property_changed,
> +					NULL, NULL);
> +
>   	return 0;
>   }
>   
> @@ -3247,5 +3599,13 @@ void __vpn_provider_cleanup(void)
>   	g_hash_table_destroy(provider_hash);
>   	provider_hash = NULL;
>   
> +	if (get_connman_state_timeout) {
> +		if (!g_source_remove(get_connman_state_timeout))
> +			connman_error("connman state timeout not removed");
> +	}
> +
> +	g_dbus_remove_watch(connection, connman_service_watch);
> +	g_dbus_remove_watch(connection, connman_signal_watch);
> +
>   	dbus_connection_unref(connection);
>   }
> 

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Daniel Wagner]

Hi Jussi,

On Thu, Aug 19, 2021 at 03:11:21PM +0300, Jussi Laakkonen wrote:
> Sorry, I spotted some error with this as apparently our fork has something
> again done differently somewhere. I'll send another version when I find the
> cause. Does not seem to function correctly on top of 1.40.

No worries.

BTW, the question on the busy loop. I am pretty sure there is no timeout
involved if you just return 'continue'. This tells the caller to queue
up the callback again without any delay. At least this is how I remember
it.

Daniel

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Santtu Lakkala]

On 19.8.2021 15.30, Daniel Wagner wrote:
> BTW, the question on the busy loop. I am pretty sure there is no timeout
> involved if you just return 'continue'. This tells the caller to queue
> up the callback again without any delay.

Nope, it called again after the same interval that was passed during 
construction.

-- 
Santtu

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Daniel Wagner]

On Thu, Aug 19, 2021 at 03:38:08PM +0300, Santtu Lakkala wrote:
> On 19.8.2021 15.30, Daniel Wagner wrote:
> > BTW, the question on the busy loop. I am pretty sure there is no timeout
> > involved if you just return 'continue'. This tells the caller to queue
> > up the callback again without any delay.
> 
> Nope, it called again after the same interval that was passed during
> construction.

Ah, okay, faulty memory here it seems. Anyway, thanks for
explaining.

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Hi Daniel,

On 8/19/21 3:30 PM, Daniel Wagner wrote:
> Hi Jussi,
> 
> On Thu, Aug 19, 2021 at 03:11:21PM +0300, Jussi Laakkonen wrote:
>> Sorry, I spotted some error with this as apparently our fork has something
>> again done differently somewhere. I'll send another version when I find the
>> cause. Does not seem to function correctly on top of 1.40.
> 
> No worries.
> 
> BTW, the question on the busy loop. I am pretty sure there is no timeout
> involved if you just return 'continue'. This tells the caller to queue
> up the callback again without any delay. At least this is how I remember
> it.
> 

I'm fairly confident in that the function is just returned back to the 
normal schedule when returning G_SOURCE_CONTINUE/TRUE.

But the issue is about VPN agent usage actually which is caused by the 
different - and quite possibly invalid use of VPN agent in our case. We 
always have an agent connected so that is our OS specific case which 
after all these years I completely forgot. But after checking with AntiX 
the issue is completely different. I need to dig into that again.

Cheers,
  Jussi

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Daniel Wagner]

On Thu, Aug 19, 2021 at 03:40:04PM +0300, Jussi Laakkonen wrote:
> But the issue is about VPN agent usage actually which is caused by the
> different - and quite possibly invalid use of VPN agent in our case. We
> always have an agent connected so that is our OS specific case which after
> all these years I completely forgot. But after checking with AntiX the issue
> is completely different. I need to dig into that again.

So the patches are no needed?

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Hi Daniel,

On 8/29/21 9:27 PM, Daniel Wagner wrote:
> On Thu, Aug 19, 2021 at 03:40:04PM +0300, Jussi Laakkonen wrote:
>> But the issue is about VPN agent usage actually which is caused by the
>> different - and quite possibly invalid use of VPN agent in our case. We
>> always have an agent connected so that is our OS specific case which after
>> all these years I completely forgot. But after checking with AntiX the issue
>> is completely different. I need to dig into that again.
> 
> So the patches are no needed?
> 

Sorry for the confusion. I think they're needed in order to let vpnd 
know the state connmand is in. The issue was in relying a VPN agent 
being present at all times which was a misunderstanding from our behalf 
because in our case VPNs cannot be connected without an agent.

So yes, I've updated the patches [PATCH v3 1/2] and [PATCH v2 2/2] last 
week, as I took another look on the old code noticed some issues to fix 
as well, which are included in the patches.

Do you want me to send them as one new patch set again or do you have 
some comments on the contents of the patches?

Cheers,
  Jussi

Re: [PATCH v2 1/2] vpn-provider: Implement connmand online state checking

[Daniel Wagner]

Hi Jussi,

On Mon, Aug 30, 2021 at 09:59:59AM +0300, Jussi Laakkonen wrote:
> Sorry for the confusion. I think they're needed in order to let vpnd know
> the state connmand is in. The issue was in relying a VPN agent being present
> at all times which was a misunderstanding from our behalf because in our
> case VPNs cannot be connected without an agent.

Alright, I'll give the patches a quick test with my VPN use cases
later, just to make sure.

> So yes, I've updated the patches [PATCH v3 1/2] and [PATCH v2 2/2] last
> week, as I took another look on the old code noticed some issues to fix as
> well, which are included in the patches.

Ah okay.

> Do you want me to send them as one new patch set again or do you have some
> comments on the contents of the patches?

The look good IIRC. But yes please, send them as new patches again. If
possible not attached to this thread, this makes it way simpler to pick
them up from the mailing list.

Daniel

[PATCH v3 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Add a complete mechanism to track connmand state and query it if
necessary to avoid connecting VPNs when there is either no connmand or
no network to use. This also makes VPNs to disconnect when connmand
loses its online state or disappears.

The connmand state listener uses net.connman.Manager interface to get
the state using GetProperties at startup. PropertyChanged signal is
monitored for state changes to update the state. State is changed and
queried when the D-Bus service listener is notified. Connmand state is
tracked within vpnd with a boolean: "true" = online/ready, "false" =
offline/idle.

Also a feature to support delayed connecting of VPNs is added. It may
happen in a situation where ConnMan status is not queried yet and
a request to connect is received over D-Bus. In this case a timeout
function is added to the main loop that runs with 1s interval. When the
delayed connect function is running it keeps on trying until connmand
state is online/ready. If connection request comes when the state is
queried and it is not online/ready ENOLINK is returned as an error
("NoCarrier" D-Bus msg). The delayed connect function is removed if
connect request comes when the function is waiting to be scheduled and
the VPN in question is connected immediately.
---
Changes since v2:
 * Cleanups and fixes to follow coding style.
 * Refactor run_get_connman_state().
 * Replace g_try_new0() with g_new().
Changes since v3:
 * Remove VPN agent checks as each VPN should do it when agent is needed.
 * Favor immediate connection of VPN if delayed connection func exists.

 vpn/vpn-provider.c | 357 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 356 insertions(+), 1 deletion(-)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 59c805c5..0ba840d3 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -3,7 +3,7 @@
  *  ConnMan VPN daemon
  *
  *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
- *  Copyright (C) 2019  Jolla Ltd. All rights reserved.
+ *  Copyright (C) 2019-2021  Jolla Ltd. All rights reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -24,6 +24,13 @@
 #include <config.h>
 #endif
 
+#define STATE_INTERVAL_DEFAULT		0
+
+#define CONNMAN_STATE_ONLINE 		"online"
+#define CONNMAN_STATE_OFFLINE 		"offline"
+#define CONNMAN_STATE_READY 		"ready"
+#define CONNMAN_STATE_IDLE 		"idle"
+
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
@@ -89,15 +96,55 @@ struct vpn_provider {
 	struct connman_ipaddress *prev_ipv4_addr;
 	struct connman_ipaddress *prev_ipv6_addr;
 	void *plugin_data;
+	unsigned int do_connect_timeout;
 	unsigned int auth_error_counter;
 	unsigned int conn_error_counter;
 	unsigned int signal_watch;
 };
 
+struct vpn_provider_connect_data {
+	DBusConnection *conn;
+	DBusMessage *msg;
+	struct vpn_provider *provider;
+};
+
+static unsigned int get_connman_state_timeout = 0;
+
+static guint connman_signal_watch;
+static guint connman_service_watch;
+
+static bool connman_online = false;
+static bool state_query_completed = false;
+
 static void append_properties(DBusMessageIter *iter,
 				struct vpn_provider *provider);
 static int vpn_provider_save(struct vpn_provider *provider);
 
+static void get_connman_state();
+
+static void set_state(const char* new_state)
+{
+	if (!new_state || !*new_state)
+		return;
+
+	DBG("old state %s new state %s",
+				connman_online ?
+				CONNMAN_STATE_ONLINE "/" CONNMAN_STATE_READY :
+				CONNMAN_STATE_OFFLINE "/" CONNMAN_STATE_IDLE,
+				new_state);
+
+	/* States "online" and "ready" mean connman is online */
+	if (!g_ascii_strcasecmp(new_state, CONNMAN_STATE_ONLINE) ||
+			!g_ascii_strcasecmp(new_state, CONNMAN_STATE_READY))
+		connman_online = true;
+	/* Everything else means connman is offline */
+	else
+		connman_online = false;
+
+	DBG("set state %s connman_online=%s ", new_state,
+				connman_online ? "true" : "false");
+}
+
 static void free_route(gpointer data)
 {
 	struct vpn_route *route = data;
@@ -740,6 +787,61 @@ static DBusMessage *clear_property(DBusConnection *conn, DBusMessage *msg,
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
+static gboolean do_connect_timeout_function(gpointer data)
+{
+	struct vpn_provider_connect_data *cdata = data;
+	struct vpn_provider *provider = cdata->provider;
+	int err;
+
+	DBG("");
+
+	/* Keep in main loop if connman is not online. */
+	if (!connman_online)
+		return G_SOURCE_CONTINUE;
+
+	provider->do_connect_timeout = 0;
+	err = __vpn_provider_connect(provider, cdata->msg);
+
+	if (err < 0 && err != -EINPROGRESS) {
+		g_dbus_send_message(cdata->conn,
+				__connman_error_failed(cdata->msg, -err));
+		cdata->msg = NULL;
+	}
+
+	return G_SOURCE_REMOVE;
+}
+
+static void do_connect_timeout_free(gpointer data)
+{
+	struct vpn_provider_connect_data *cdata = data;
+
+	if (cdata->msg)
+		g_dbus_send_message(cdata->conn,
+				__connman_error_operation_aborted(cdata->msg));
+
+	dbus_connection_unref(cdata->conn);
+	g_free(data);
+}
+
+static void do_connect_later(struct vpn_provider *provider,
+					DBusConnection *conn, DBusMessage *msg)
+{
+	struct vpn_provider_connect_data *cdata =
+				g_new0(struct vpn_provider_connect_data, 1);
+
+	cdata->conn = dbus_connection_ref(conn);
+	cdata->msg = dbus_message_ref(msg);
+	cdata->provider = provider;
+
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
+	provider->do_connect_timeout =
+			g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
+					do_connect_timeout_function, cdata,
+					do_connect_timeout_free);
+}
+
 static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 								void *data)
 {
@@ -748,6 +850,27 @@ static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 
 	DBG("conn %p provider %p", conn, provider);
 
+	if (!connman_online) {
+		if (state_query_completed) {
+			DBG("Provider %s not started because connman "
+						"not online/ready",
+						provider->identifier);
+			return __connman_error_failed(msg, ENOLINK);
+		} else {
+			DBG("Provider %s start delayed because connman "
+						"state not queried",
+						provider->identifier);
+			do_connect_later(provider, conn, msg);
+			return NULL;
+		}
+	}
+
+	/* Cancel delayed connection if connmand is online. */
+	if (provider->do_connect_timeout) {
+		g_source_remove(provider->do_connect_timeout);
+		provider->do_connect_timeout = 0;
+	}
+
 	err = __vpn_provider_connect(provider, msg);
 	if (err < 0 && err != -EINPROGRESS)
 		return __connman_error_failed(msg, -err);
@@ -1269,6 +1392,9 @@ static void provider_destruct(struct vpn_provider *provider)
 {
 	DBG("provider %p", provider);
 
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
 	if (provider->notify_id != 0)
 		g_source_remove(provider->notify_id);
 
@@ -3214,6 +3340,213 @@ static void remove_unprovisioned_providers(void)
 	g_strfreev(providers);
 }
 
+static gboolean connman_property_changed(DBusConnection *conn,
+				DBusMessage *message,
+				void *user_data)
+{
+	DBusMessageIter iter, value;
+	const char *key;
+	const char *signature = DBUS_TYPE_STRING_AS_STRING
+				DBUS_TYPE_VARIANT_AS_STRING;
+
+	if (!dbus_message_has_signature(message, signature)) {
+		connman_error("vpn connman property signature \"%s\" "
+					"does not match expected \"%s\"",
+					dbus_message_get_signature(message),
+					signature);
+		return TRUE;
+	}
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+
+	dbus_message_iter_next(&iter);
+	dbus_message_iter_recurse(&iter, &value);
+
+	DBG("key %s", key);
+
+	if (g_str_equal(key, "State")) {
+		const char *str;
+
+		if (dbus_message_iter_get_arg_type(&value) != DBUS_TYPE_STRING)
+			return TRUE;
+
+		dbus_message_iter_get_basic(&value, &str);
+		set_state(str);
+	}
+
+	return TRUE;
+}
+
+static void get_connman_state_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply = NULL;
+	DBusError error;
+	DBusMessageIter iter, array, dict, value;
+
+	const char *signature = DBUS_TYPE_ARRAY_AS_STRING
+				DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+				DBUS_TYPE_STRING_AS_STRING
+				DBUS_TYPE_VARIANT_AS_STRING
+				DBUS_DICT_ENTRY_END_CHAR_AS_STRING;
+
+	const char *key;
+	const char *str;
+
+	DBG("");
+
+	if (!dbus_pending_call_get_completed(call))
+		goto done;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&error);
+
+	if (dbus_set_error_from_message(&error, reply)) {
+		connman_error("%s", error.message);
+
+		/*
+		 * In case of timeout re-add the state function to main
+		 * event loop.
+		 */
+		if (g_ascii_strcasecmp(error.name, DBUS_ERROR_TIMEOUT) == 0) {
+			DBG("D-Bus timeout, re-add get_connman_state()");
+			get_connman_state();
+		} else {
+			dbus_error_free(&error);
+			goto done;
+		}
+	}
+
+	if (!dbus_message_has_signature(reply, signature)) {
+		connman_error("vpnd signature \"%s\" does not match "
+					"expected \"%s\"",
+					dbus_message_get_signature(reply),
+					signature);
+		goto done;
+	}
+
+	if (!dbus_message_iter_init(reply, &array))
+		goto done;
+
+	dbus_message_iter_recurse(&array, &dict);
+
+	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
+		dbus_message_iter_recurse(&dict, &iter);
+
+		dbus_message_iter_get_basic(&iter, &key);
+
+		dbus_message_iter_next(&iter);
+		dbus_message_iter_recurse(&iter, &value);
+
+		if (g_ascii_strcasecmp(key, "State") == 0 &&
+				dbus_message_iter_get_arg_type(&value) ==
+						DBUS_TYPE_STRING) {
+			dbus_message_iter_get_basic(&value, &str);
+
+			DBG("Got initial state %s", str);
+
+			set_state(str);
+
+			/* No need to process further */
+			break;
+		}
+
+		dbus_message_iter_next(&dict);
+	}
+
+	state_query_completed = true;
+
+done:
+	if (reply)
+		dbus_message_unref(reply);
+
+	if (call)
+		dbus_pending_call_unref(call);
+}
+
+static gboolean run_get_connman_state(gpointer user_data)
+{
+	const char *path = "/";
+	const char *method = "GetProperties";
+	gboolean rval = FALSE;
+
+	DBusMessage *msg = NULL;
+	DBusPendingCall *call = NULL;
+
+	DBG("");
+
+	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
+					CONNMAN_MANAGER_INTERFACE, method);
+	if (!msg)
+		goto out;
+
+	rval = g_dbus_send_message_with_reply(connection, msg, &call, -1);
+	if (!rval) {
+		connman_error("Cannot call %s on %s", method,
+						CONNMAN_MANAGER_INTERFACE);
+		goto out;
+	}
+
+	if (!call) {
+		connman_error("set pending call failed");
+		rval = FALSE;
+		goto out;
+	}
+
+	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
+								NULL, NULL)) {
+		connman_error("set notify to pending call failed");
+
+		if (call)
+			dbus_pending_call_unref(call);
+
+		rval = FALSE;
+	}
+
+out:
+	if (msg)
+		dbus_message_unref(msg);
+
+	/* In case sending was success, unset timeout function id */
+	if (rval) {
+		DBG("unsetting get_connman_state_timeout id");
+		get_connman_state_timeout = 0;
+	}
+
+	/* Return FALSE in case of success to remove from main event loop */
+	return !rval;
+}
+
+static void get_connman_state()
+{
+	if (get_connman_state_timeout)
+		return;
+
+	get_connman_state_timeout = g_timeout_add(STATE_INTERVAL_DEFAULT,
+						run_get_connman_state, NULL);
+}
+
+static void connman_service_watch_connected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+	get_connman_state();
+}
+
+static void connman_service_watch_disconnected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+
+	set_state(CONNMAN_STATE_IDLE);
+
+	/* Set state query variable to initial state */
+	state_query_completed = false;
+}
+
 int __vpn_provider_init(void)
 {
 	int err;
@@ -3233,6 +3566,20 @@ int __vpn_provider_init(void)
 
 	provider_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
 						NULL, unregister_provider);
+
+	connman_service_watch = g_dbus_add_service_watch(connection,
+					CONNMAN_SERVICE,
+					connman_service_watch_connected,
+					connman_service_watch_disconnected,
+					NULL, NULL);
+
+	connman_signal_watch = g_dbus_add_signal_watch(connection,
+					CONNMAN_SERVICE, NULL,
+					CONNMAN_MANAGER_INTERFACE,
+					PROPERTY_CHANGED,
+					connman_property_changed,
+					NULL, NULL);
+
 	return 0;
 }
 
@@ -3247,5 +3594,13 @@ void __vpn_provider_cleanup(void)
 	g_hash_table_destroy(provider_hash);
 	provider_hash = NULL;
 
+	if (get_connman_state_timeout) {
+		if (!g_source_remove(get_connman_state_timeout))
+			connman_error("connman state timeout not removed");
+	}
+
+	g_dbus_remove_watch(connection, connman_service_watch);
+	g_dbus_remove_watch(connection, connman_signal_watch);
+
 	dbus_connection_unref(connection);
 }
-- 
2.20.1

[PATCH 2/2] vpn: Handle ENOLINK error in connect callback

[Jussi Laakkonen]

Add handling of ENOLINK error that is reported back by vpnd when a VPN
cannot be connected because connmand is in offline state. ENOLINK has
to be handled similarly to EINPROGRESS as it is not a real error in the
actual VPN connection.
---
 plugins/vpn.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/plugins/vpn.c b/plugins/vpn.c
index d708d1ff..bc025c0c 100644
--- a/plugins/vpn.c
+++ b/plugins/vpn.c
@@ -491,6 +491,9 @@ static int errorstr2val(const char *error) {
 	if (g_strcmp0(error, CONNMAN_ERROR_INTERFACE ".AlreadyConnected") == 0)
 		return -EISCONN;
 
+	if (g_strcmp0(error, CONNMAN_ERROR_INTERFACE ".NoCarrier") == 0)
+		return -ENOLINK;
+
 	if (g_strcmp0(error, CONNMAN_ERROR_INTERFACE ".OperationCanceled") == 0)
 		return -ECANCELED;
 
@@ -538,7 +541,12 @@ static void connect_reply(DBusPendingCall *call, void *user_data)
 		if (err == -ECANCELED) {
 			DBG("%s connect canceled", data->path);
 			connman_provider_set_autoconnect(data->provider, false);
-		} else if (err != -EINPROGRESS) {
+		/*
+		 * ENOLINK (No carrier) is not an error situation but is caused
+		 * by connman not being online when VPN is attempted to be
+		 * connected.
+		 */
+		} else if (err != -EINPROGRESS && err != -ENOLINK) {
 			connman_error("Connect reply: %s (%s)", error.message,
 								error.name);
 			DBG("data %p cb_data %p", data, cb_data);
-- 
2.20.1

[PATCH v2 2/2] vpn: Refactor connect_reply() and handle NoCarrier -> ENOLINK error

[Jussi Laakkonen]

Refactor connect_reply() to be extendable for more fine grained error
handling.

Add handling of ENOLINK error that is reported back by vpnd when a VPN
cannot be connected because connmand is in offline state. ENOLINK is to
be handled as any other error causing the VPN state not to change later
on that in turn would cause the cb_data->callback() never getting
called.
---
Changes since v2:
 * Change commit title to describe the change.
 * ENOLINK must be handled as an error since otherwise callback is not called
 * Refactor the connect_reply() to be more extendable in the future.

 plugins/vpn.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/plugins/vpn.c b/plugins/vpn.c
index d708d1ff..387447c3 100644
--- a/plugins/vpn.c
+++ b/plugins/vpn.c
@@ -491,6 +491,9 @@ static int errorstr2val(const char *error) {
 	if (g_strcmp0(error, CONNMAN_ERROR_INTERFACE ".AlreadyConnected") == 0)
 		return -EISCONN;
 
+	if (g_strcmp0(error, CONNMAN_ERROR_INTERFACE ".NoCarrier") == 0)
+		return -ENOLINK;
+
 	if (g_strcmp0(error, CONNMAN_ERROR_INTERFACE ".OperationCanceled") == 0)
 		return -ECANCELED;
 
@@ -529,16 +532,23 @@ static void connect_reply(DBusPendingCall *call, void *user_data)
 	if (dbus_set_error_from_message(&error, reply)) {
 		int err = errorstr2val(error.name);
 
+		switch (err) {
+		case -EINPROGRESS:
+			break;
 		/*
 		 * ECANCELED means that user has canceled authentication
 		 * dialog. That's not really an error, it's part of a normal
 		 * workflow. We also take it as a request to turn autoconnect
 		 * off, in case if it was on.
 		 */
-		if (err == -ECANCELED) {
+		case -ECANCELED:
 			DBG("%s connect canceled", data->path);
 			connman_provider_set_autoconnect(data->provider, false);
-		} else if (err != -EINPROGRESS) {
+			break;
+		case -ENOLINK: /* vpnd reports that connmand is not online. */
+		case -EISCONN:
+		case -ECONNREFUSED:
+		default:
 			connman_error("Connect reply: %s (%s)", error.message,
 								error.name);
 			DBG("data %p cb_data %p", data, cb_data);
-- 
2.20.1

[PATCH v2 0/2] Implement connmand state monitoring to vpnd

[Jussi Laakkonen]

Implement connmand state monitoring into vpnd to keep track of the state
changes in regards to connectivity and D-Bus registration. vpnd retrieves the
state from connmand using Manager interface when started and otherwise it
monitors PropertyChanged signals as well as utilizes D-Bus service monitoring
to get the changes.

This change prevents connecting VPNs when connmand is not yet online or has
gone away from D-Bus (e.g., restart). In case VPN is connected before the state
has been queried a delayed connection function is utilized to connect that VPN
when online/ready state is reached.

In case connmand is offline or gone from D-Bus ENOLINK is returned as an error.
This is handled in plugins/vpn.c and returned back to the caller as NoCarrier
D-Bus error message.

Changes since V2:
 - Updated vpn-provider patch to v3: cleanups, reformat, remove agent checks.
 - Updated vpn patch to v2: do handle ENOLINK properly and reformat.

Jussi Laakkonen (2):
  vpn-provider: Implement connmand online state checking
  vpn: Refactor connect_reply() and handle NoCarrier ->  ENOLINK error

 plugins/vpn.c      |  14 +-
 vpn/vpn-provider.c | 357 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 368 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH v3 1/2] vpn-provider: Implement connmand online state checking

[Jussi Laakkonen]

Add a complete mechanism to track connmand state and query it if
necessary to avoid connecting VPNs when there is either no connmand or
no network to use. This also makes VPNs to disconnect when connmand
loses its online state or disappears.

The connmand state listener uses net.connman.Manager interface to get
the state using GetProperties at startup. PropertyChanged signal is
monitored for state changes to update the state. State is changed and
queried when the D-Bus service listener is notified. Connmand state is
tracked within vpnd with a boolean: "true" = online/ready, "false" =
offline/idle.

Also a feature to support delayed connecting of VPNs is added. It may
happen in a situation where ConnMan status is not queried yet and
a request to connect is received over D-Bus. In this case a timeout
function is added to the main loop that runs with 1s interval. When the
delayed connect function is running it keeps on trying until connmand
state is online/ready. If connection request comes when the state is
queried and it is not online/ready ENOLINK is returned as an error
("NoCarrier" D-Bus msg). The delayed connect function is removed if
connect request comes when the function is waiting to be scheduled and
the VPN in question is connected immediately.
---
Changes since v2:
 * Cleanups and fixes to follow coding style.
 * Refactor run_get_connman_state().
 * Replace g_try_new0() with g_new().
Changes since v3:
 * Remove VPN agent checks as each VPN should do it when agent is needed.
 * Favor immediate connection of VPN if delayed connection func exists.

 vpn/vpn-provider.c | 357 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 356 insertions(+), 1 deletion(-)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 59c805c5..0ba840d3 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -3,7 +3,7 @@
  *  ConnMan VPN daemon
  *
  *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
- *  Copyright (C) 2019  Jolla Ltd. All rights reserved.
+ *  Copyright (C) 2019-2021  Jolla Ltd. All rights reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -24,6 +24,13 @@
 #include <config.h>
 #endif
 
+#define STATE_INTERVAL_DEFAULT		0
+
+#define CONNMAN_STATE_ONLINE 		"online"
+#define CONNMAN_STATE_OFFLINE 		"offline"
+#define CONNMAN_STATE_READY 		"ready"
+#define CONNMAN_STATE_IDLE 		"idle"
+
 #include <errno.h>
 #include <stdio.h>
 #include <string.h>
@@ -89,15 +96,55 @@ struct vpn_provider {
 	struct connman_ipaddress *prev_ipv4_addr;
 	struct connman_ipaddress *prev_ipv6_addr;
 	void *plugin_data;
+	unsigned int do_connect_timeout;
 	unsigned int auth_error_counter;
 	unsigned int conn_error_counter;
 	unsigned int signal_watch;
 };
 
+struct vpn_provider_connect_data {
+	DBusConnection *conn;
+	DBusMessage *msg;
+	struct vpn_provider *provider;
+};
+
+static unsigned int get_connman_state_timeout = 0;
+
+static guint connman_signal_watch;
+static guint connman_service_watch;
+
+static bool connman_online = false;
+static bool state_query_completed = false;
+
 static void append_properties(DBusMessageIter *iter,
 				struct vpn_provider *provider);
 static int vpn_provider_save(struct vpn_provider *provider);
 
+static void get_connman_state();
+
+static void set_state(const char* new_state)
+{
+	if (!new_state || !*new_state)
+		return;
+
+	DBG("old state %s new state %s",
+				connman_online ?
+				CONNMAN_STATE_ONLINE "/" CONNMAN_STATE_READY :
+				CONNMAN_STATE_OFFLINE "/" CONNMAN_STATE_IDLE,
+				new_state);
+
+	/* States "online" and "ready" mean connman is online */
+	if (!g_ascii_strcasecmp(new_state, CONNMAN_STATE_ONLINE) ||
+			!g_ascii_strcasecmp(new_state, CONNMAN_STATE_READY))
+		connman_online = true;
+	/* Everything else means connman is offline */
+	else
+		connman_online = false;
+
+	DBG("set state %s connman_online=%s ", new_state,
+				connman_online ? "true" : "false");
+}
+
 static void free_route(gpointer data)
 {
 	struct vpn_route *route = data;
@@ -740,6 +787,61 @@ static DBusMessage *clear_property(DBusConnection *conn, DBusMessage *msg,
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
+static gboolean do_connect_timeout_function(gpointer data)
+{
+	struct vpn_provider_connect_data *cdata = data;
+	struct vpn_provider *provider = cdata->provider;
+	int err;
+
+	DBG("");
+
+	/* Keep in main loop if connman is not online. */
+	if (!connman_online)
+		return G_SOURCE_CONTINUE;
+
+	provider->do_connect_timeout = 0;
+	err = __vpn_provider_connect(provider, cdata->msg);
+
+	if (err < 0 && err != -EINPROGRESS) {
+		g_dbus_send_message(cdata->conn,
+				__connman_error_failed(cdata->msg, -err));
+		cdata->msg = NULL;
+	}
+
+	return G_SOURCE_REMOVE;
+}
+
+static void do_connect_timeout_free(gpointer data)
+{
+	struct vpn_provider_connect_data *cdata = data;
+
+	if (cdata->msg)
+		g_dbus_send_message(cdata->conn,
+				__connman_error_operation_aborted(cdata->msg));
+
+	dbus_connection_unref(cdata->conn);
+	g_free(data);
+}
+
+static void do_connect_later(struct vpn_provider *provider,
+					DBusConnection *conn, DBusMessage *msg)
+{
+	struct vpn_provider_connect_data *cdata =
+				g_new0(struct vpn_provider_connect_data, 1);
+
+	cdata->conn = dbus_connection_ref(conn);
+	cdata->msg = dbus_message_ref(msg);
+	cdata->provider = provider;
+
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
+	provider->do_connect_timeout =
+			g_timeout_add_seconds_full(G_PRIORITY_DEFAULT, 1,
+					do_connect_timeout_function, cdata,
+					do_connect_timeout_free);
+}
+
 static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 								void *data)
 {
@@ -748,6 +850,27 @@ static DBusMessage *do_connect(DBusConnection *conn, DBusMessage *msg,
 
 	DBG("conn %p provider %p", conn, provider);
 
+	if (!connman_online) {
+		if (state_query_completed) {
+			DBG("Provider %s not started because connman "
+						"not online/ready",
+						provider->identifier);
+			return __connman_error_failed(msg, ENOLINK);
+		} else {
+			DBG("Provider %s start delayed because connman "
+						"state not queried",
+						provider->identifier);
+			do_connect_later(provider, conn, msg);
+			return NULL;
+		}
+	}
+
+	/* Cancel delayed connection if connmand is online. */
+	if (provider->do_connect_timeout) {
+		g_source_remove(provider->do_connect_timeout);
+		provider->do_connect_timeout = 0;
+	}
+
 	err = __vpn_provider_connect(provider, msg);
 	if (err < 0 && err != -EINPROGRESS)
 		return __connman_error_failed(msg, -err);
@@ -1269,6 +1392,9 @@ static void provider_destruct(struct vpn_provider *provider)
 {
 	DBG("provider %p", provider);
 
+	if (provider->do_connect_timeout)
+		g_source_remove(provider->do_connect_timeout);
+
 	if (provider->notify_id != 0)
 		g_source_remove(provider->notify_id);
 
@@ -3214,6 +3340,213 @@ static void remove_unprovisioned_providers(void)
 	g_strfreev(providers);
 }
 
+static gboolean connman_property_changed(DBusConnection *conn,
+				DBusMessage *message,
+				void *user_data)
+{
+	DBusMessageIter iter, value;
+	const char *key;
+	const char *signature = DBUS_TYPE_STRING_AS_STRING
+				DBUS_TYPE_VARIANT_AS_STRING;
+
+	if (!dbus_message_has_signature(message, signature)) {
+		connman_error("vpn connman property signature \"%s\" "
+					"does not match expected \"%s\"",
+					dbus_message_get_signature(message),
+					signature);
+		return TRUE;
+	}
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+
+	dbus_message_iter_next(&iter);
+	dbus_message_iter_recurse(&iter, &value);
+
+	DBG("key %s", key);
+
+	if (g_str_equal(key, "State")) {
+		const char *str;
+
+		if (dbus_message_iter_get_arg_type(&value) != DBUS_TYPE_STRING)
+			return TRUE;
+
+		dbus_message_iter_get_basic(&value, &str);
+		set_state(str);
+	}
+
+	return TRUE;
+}
+
+static void get_connman_state_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply = NULL;
+	DBusError error;
+	DBusMessageIter iter, array, dict, value;
+
+	const char *signature = DBUS_TYPE_ARRAY_AS_STRING
+				DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+				DBUS_TYPE_STRING_AS_STRING
+				DBUS_TYPE_VARIANT_AS_STRING
+				DBUS_DICT_ENTRY_END_CHAR_AS_STRING;
+
+	const char *key;
+	const char *str;
+
+	DBG("");
+
+	if (!dbus_pending_call_get_completed(call))
+		goto done;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&error);
+
+	if (dbus_set_error_from_message(&error, reply)) {
+		connman_error("%s", error.message);
+
+		/*
+		 * In case of timeout re-add the state function to main
+		 * event loop.
+		 */
+		if (g_ascii_strcasecmp(error.name, DBUS_ERROR_TIMEOUT) == 0) {
+			DBG("D-Bus timeout, re-add get_connman_state()");
+			get_connman_state();
+		} else {
+			dbus_error_free(&error);
+			goto done;
+		}
+	}
+
+	if (!dbus_message_has_signature(reply, signature)) {
+		connman_error("vpnd signature \"%s\" does not match "
+					"expected \"%s\"",
+					dbus_message_get_signature(reply),
+					signature);
+		goto done;
+	}
+
+	if (!dbus_message_iter_init(reply, &array))
+		goto done;
+
+	dbus_message_iter_recurse(&array, &dict);
+
+	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
+		dbus_message_iter_recurse(&dict, &iter);
+
+		dbus_message_iter_get_basic(&iter, &key);
+
+		dbus_message_iter_next(&iter);
+		dbus_message_iter_recurse(&iter, &value);
+
+		if (g_ascii_strcasecmp(key, "State") == 0 &&
+				dbus_message_iter_get_arg_type(&value) ==
+						DBUS_TYPE_STRING) {
+			dbus_message_iter_get_basic(&value, &str);
+
+			DBG("Got initial state %s", str);
+
+			set_state(str);
+
+			/* No need to process further */
+			break;
+		}
+
+		dbus_message_iter_next(&dict);
+	}
+
+	state_query_completed = true;
+
+done:
+	if (reply)
+		dbus_message_unref(reply);
+
+	if (call)
+		dbus_pending_call_unref(call);
+}
+
+static gboolean run_get_connman_state(gpointer user_data)
+{
+	const char *path = "/";
+	const char *method = "GetProperties";
+	gboolean rval = FALSE;
+
+	DBusMessage *msg = NULL;
+	DBusPendingCall *call = NULL;
+
+	DBG("");
+
+	msg = dbus_message_new_method_call(CONNMAN_SERVICE, path,
+					CONNMAN_MANAGER_INTERFACE, method);
+	if (!msg)
+		goto out;
+
+	rval = g_dbus_send_message_with_reply(connection, msg, &call, -1);
+	if (!rval) {
+		connman_error("Cannot call %s on %s", method,
+						CONNMAN_MANAGER_INTERFACE);
+		goto out;
+	}
+
+	if (!call) {
+		connman_error("set pending call failed");
+		rval = FALSE;
+		goto out;
+	}
+
+	if (!dbus_pending_call_set_notify(call, get_connman_state_reply,
+								NULL, NULL)) {
+		connman_error("set notify to pending call failed");
+
+		if (call)
+			dbus_pending_call_unref(call);
+
+		rval = FALSE;
+	}
+
+out:
+	if (msg)
+		dbus_message_unref(msg);
+
+	/* In case sending was success, unset timeout function id */
+	if (rval) {
+		DBG("unsetting get_connman_state_timeout id");
+		get_connman_state_timeout = 0;
+	}
+
+	/* Return FALSE in case of success to remove from main event loop */
+	return !rval;
+}
+
+static void get_connman_state()
+{
+	if (get_connman_state_timeout)
+		return;
+
+	get_connman_state_timeout = g_timeout_add(STATE_INTERVAL_DEFAULT,
+						run_get_connman_state, NULL);
+}
+
+static void connman_service_watch_connected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+	get_connman_state();
+}
+
+static void connman_service_watch_disconnected(DBusConnection *conn,
+				void *user_data)
+{
+	DBG("");
+
+	set_state(CONNMAN_STATE_IDLE);
+
+	/* Set state query variable to initial state */
+	state_query_completed = false;
+}
+
 int __vpn_provider_init(void)
 {
 	int err;
@@ -3233,6 +3566,20 @@ int __vpn_provider_init(void)
 
 	provider_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
 						NULL, unregister_provider);
+
+	connman_service_watch = g_dbus_add_service_watch(connection,
+					CONNMAN_SERVICE,
+					connman_service_watch_connected,
+					connman_service_watch_disconnected,
+					NULL, NULL);
+
+	connman_signal_watch = g_dbus_add_signal_watch(connection,
+					CONNMAN_SERVICE, NULL,
+					CONNMAN_MANAGER_INTERFACE,
+					PROPERTY_CHANGED,
+					connman_property_changed,
+					NULL, NULL);
+
 	return 0;
 }
 
@@ -3247,5 +3594,13 @@ void __vpn_provider_cleanup(void)
 	g_hash_table_destroy(provider_hash);
 	provider_hash = NULL;
 
+	if (get_connman_state_timeout) {
+		if (!g_source_remove(get_connman_state_timeout))
+			connman_error("connman state timeout not removed");
+	}
+
+	g_dbus_remove_watch(connection, connman_service_watch);
+	g_dbus_remove_watch(connection, connman_signal_watch);
+
 	dbus_connection_unref(connection);
 }
-- 
2.20.1