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

[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

[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

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

[Daniel Wagner]

Hi Jussi,

On Mon, Aug 30, 2021 at 12:24:03PM +0300, Jussi Laakkonen wrote:
> 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

I fixed up some small whitespace issues in the first patch and dropped
the else block in do_connect(). Also added the 'void' argument to
get_connman_state() (all complains from checkpatch).

Also I had to fallback to my old workflow and cherrypick the patches. b4
was not happy with the thread:

  Grabbing thread from lore.kernel.org/connman/20210830092405.30964-2-jussi.laakkonen%40jolla.com/t.mbox.gz
  Will use the latest revision: v3
  You can pick other revisions using the -vN flag
  Checking attestation on all messages, may take a moment...
  ---
    [PATCH v3 1/2] vpn-provider: Implement connmand online state checking
    ERROR: missing [2/2]!
  ---

Anyway, I gave it a quick test, all looks good. Patches applied.

Thanks,
Daniel

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

[Jussi Laakkonen]

Hi Daniel,

On 8/30/21 8:06 PM, Daniel Wagner wrote:
> Hi Jussi,
> 
> On Mon, Aug 30, 2021 at 12:24:03PM +0300, Jussi Laakkonen wrote:
>> 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
> 
> I fixed up some small whitespace issues in the first patch and dropped
> the else block in do_connect(). Also added the 'void' argument to
> get_connman_state() (all complains from checkpatch).
> 

Ok, that is cool. Apparently my git did not find any whitespace issues 
with the patches as I do check that prior to sending, hmm.

> Also I had to fallback to my old workflow and cherrypick the patches. b4
> was not happy with the thread:
> 
>    Grabbing thread from lore.kernel.org/connman/20210830092405.30964-2-jussi.laakkonen%40jolla.com/t.mbox.gz
>    Will use the latest revision: v3
>    You can pick other revisions using the -vN flag
>    Checking attestation on all messages, may take a moment...
>    ---
>      [PATCH v3 1/2] vpn-provider: Implement connmand online state checking
>      ERROR: missing [2/2]!
>    ---
> 
> Anyway, I gave it a quick test, all looks good. Patches applied.
> 

Great, thanks. I'm curious, what kind of workflow you have now and what 
I should take account for prior to sending the patches. Does the b4 
require that all of those patches are in the same thread or otherwise 
reset the versioning when re-sending the patches as a new set?

Also, it wouldn't hurt to download Linus' checkpatch.pl :)

Cheers,
  Jussi

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

[Daniel Wagner]

Hi Jussi,

On Tue, Aug 31, 2021 at 10:00:40AM +0300, Jussi Laakkonen wrote:
> > I fixed up some small whitespace issues in the first patch and dropped
> > the else block in do_connect(). Also added the 'void' argument to
> > get_connman_state() (all complains from checkpatch).
> > 
> 
> Ok, that is cool. Apparently my git did not find any whitespace issues with
> the patches as I do check that prior to sending, hmm.

No worries. I am using the whitespace mode in emacs to highlight issues,
but still I manage to add random whitespace damages. At least the end of
line stuff is pretty much gone with this help.

> Great, thanks. I'm curious, what kind of workflow you have now and what I
> should take account for prior to sending the patches. Does the b4 require
> that all of those patches are in the same thread or otherwise reset the
> versioning when re-sending the patches as a new set?

b4 is really targeting towards full series with consistent version
numbers in all patches posted as new mail thread. It understands the
versioning strategies and workflow. For example if I tell it to use v2
instead of v3, it will tell me, there is a v3 on the mailing list
(subject of the cover letter has to match). Overall, it's an absolute
awesome tool and I am glad we are hosted on lists.linux.dev, so that I
can use it. Konstantin did a very nice presentation recently on b4:

  https://people.kernel.org/monsieuricon/

Anyway the idea is that you have changes in your git tree and to create
a new posting do something along the lines of:

  git format-patch HEAD~~ -o ../patches/fancy-feature -v1 --cover-letter
  [edit cover letter]
  git send-email ../patches/fancy-feature/v1* --to=connman

and after reworking the commits, you do the same just use '-v2'.

BTW, if you want to add a change log per patch, you can use 'git notes'
and use 'git format-patch --notes' to add them. I haven't used this so
far, but looks very useful. Have to play with it :)

> Also, it wouldn't hurt to download Linus' checkpatch.pl :)

Yeah, just use it with a grain of salt. It tends to be too eager but it
helps to get some consistency into the code base.

Daniel

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

[Jussi Laakkonen]

Hi Daniel,

On 8/31/21 10:22 AM, Daniel Wagner wrote:
> Hi Jussi,
> 
> No worries. I am using the whitespace mode in emacs to highlight issues,
> but still I manage to add random whitespace damages. At least the end of
> line stuff is pretty much gone with this help.
> 
>> Great, thanks. I'm curious, what kind of workflow you have now and what I
>> should take account for prior to sending the patches. Does the b4 require
>> that all of those patches are in the same thread or otherwise reset the
>> versioning when re-sending the patches as a new set?
> 
> b4 is really targeting towards full series with consistent version
> numbers in all patches posted as new mail thread. It understands the
> versioning strategies and workflow. For example if I tell it to use v2
> instead of v3, it will tell me, there is a v3 on the mailing list
> (subject of the cover letter has to match). Overall, it's an absolute
> awesome tool and I am glad we are hosted on lists.linux.dev, so that I
> can use it. Konstantin did a very nice presentation recently on b4:
> 
>    https://people.kernel.org/monsieuricon/
> 
> Anyway the idea is that you have changes in your git tree and to create
> a new posting do something along the lines of:
> 
>    git format-patch HEAD~~ -o ../patches/fancy-feature -v1 --cover-letter
>    [edit cover letter]
>    git send-email ../patches/fancy-feature/v1* --to=connman
> 
> and after reworking the commits, you do the same just use '-v2'.
> 
> BTW, if you want to add a change log per patch, you can use 'git notes'
> and use 'git format-patch --notes' to add them. I haven't used this so
> far, but looks very useful. Have to play with it :)
> 
>> Also, it wouldn't hurt to download Linus' checkpatch.pl :)
> 
> Yeah, just use it with a grain of salt. It tends to be too eager but it
> helps to get some consistency into the code base.
> 

Thanks for the info, I really need to look into that b4 as well. Yeah, 
that notes does seem to be useful, I've just edited the commits manually 
as of now.

Some old whitespace errors can be easily overlooked and that kind of 
checker-script just saves a lot of time. Glad to learn new ways once in 
a while.

Cheers,
  Jussi

[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