stemodem: Add RTNL functionality managing CAIF Network Interfaces.

 drivers/stemodem/gprs-context.c |  207 +++++++++++++++++++++++++--------------
 1 files changed, 135 insertions(+), 72 deletions(-)

drivers/stemodem/gprs-context.c +1 -0

index 9f59579..4260d31 100644
@@ -28,6 +28,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <errno.h>
 
 #include <glib.h>
 

drivers/stemodem/gprs-context.c +2 -1

 #define MAX_DNS 2
-#define MAX_ELEM 20
+#define IP_ADDR_LEN 20
 
 #define AUTH_BUF_LENGTH (OFONO_GPRS_MAX_USERNAME_LENGTH + \
 			OFONO_GPRS_MAX_PASSWORD_LENGTH + 128)

@@ -65,21 +67,29 @@ struct gprs_context_data {
 };
 
 struct conn_info {
+	/*

drivers/stemodem/gprs-context.c +3 -1

+	/* Linux Interface name */
+	char interface[IF_NAMESIZE];
+	gboolean created;
 };
 
 struct eppsd_response {
 	char *current;
-	char ip_address[MAX_ELEM];

drivers/stemodem/gprs-context.c +3 -0

+	char dns_server1[IP_ADDR_LEN];
+	char dns_server2[IP_ADDR_LEN];
+	char p_cscf_server[IP_ADDR_LEN];
 };
 
 static void start_element_handler(GMarkupParseContext *context,

drivers/stemodem/gprs-context.c +6 -3

 	}
 }
 

@@ -153,8 +163,7 @@ static gint conn_compare_by_cid(gconstpointer a, gconstpointer b)
 	return 0;
 }
 
-static struct conn_info *conn_info_create(unsigned int device,
-						unsigned int channel_id)
+static struct conn_info *conn_info_create(unsigned int channel_id)
 {
 	struct conn_info *connection = g_try_new0(struct conn_info, 1);
 

@@ -162,26 +171,61 @@ static struct conn_info *conn_info_create(unsigned int device,
 		return NULL;
 
 	connection->cid = 0;
-	connection->device = device;
 	connection->channel_id = channel_id;
 
 	return connection;
 }
 
+static void rtnl_callback(int result, gpointer user_data,
+		char *ifname, int ifindex)
+{
+	struct conn_info *conn = user_data;
+

drivers/stemodem/gprs-context.c +94 -56

+
 /*
  * Creates a new IP interface for CAIF.
  */
-static gboolean caif_if_create(const char *interface, unsigned int connid)
+static gboolean caif_if_create(struct conn_info *conn)
 {
-	return FALSE;
+	int err;
+
+	err = caif_rtnl_create_interface(conn, IFLA_CAIF_IPV4_CONNID,
+					conn->channel_id, FALSE, rtnl_callback);
+	if (err < 0) {
+		DBG("Failed to create IP interface for CAIF");
+		return FALSE;
+	}
+
+	return TRUE;
 }
 
 /*
  * Removes IP interface for CAIF.
  */
-static gboolean caif_if_remove(const char *interface, unsigned int connid)
+static void caif_if_remove(struct conn_info *conn)
 {
-	return FALSE;
+	if (!conn->created)
+		return;
+
+	if (caif_rtnl_delete_interface(conn->ifindex) < 0) {
+		ofono_error("Failed to delete caif interface %s",
+			conn->interface);
+		return;
+	}
+
+	DBG("removed CAIF interface ch:%d ifname:%s ifindex:%d\n",
+		conn->channel_id, conn->interface, conn->ifindex);
+	return;
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,

@@ -192,11 +236,14 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
 	struct ofono_gprs_context *gc = cbd->user;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 	struct ofono_error error;
-	struct conn_info *conn;
+	struct conn_info *conn = NULL;
 	GSList *l;
 
-	if (!ok)
-		goto error;
+	if (!ok) {
+		decode_at_error(&error, g_at_result_final_response(result));
+		cb(&error, cbd->data);
+		return;
+	}
 
 	l = g_slist_find_custom(g_caif_devices,
 				GUINT_TO_POINTER(gcd->active_context),

@@ -210,16 +257,8 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
 	}
 
 	conn = l->data;
-
-	if (!caif_if_remove(conn->interface, conn->channel_id)) {
-		DBG("Failed to remove caif interface %s.",
-				conn->interface);
-	}
-
 	conn->cid = 0;
-
-	decode_at_error(&error, g_at_result_final_response(result));
-	cb(&error, cbd->data);
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 	return;
 
 error:

@@ -237,26 +276,30 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	GSList *l;
 	int i;
 	gsize length;
-	char *res_string;
+	const char *res_string;
 	const char *dns[MAX_DNS + 1];
 	struct eppsd_response rsp;
 	GMarkupParseContext *context = NULL;
+	struct ofono_error error;
 
 	l = g_slist_find_custom(g_caif_devices,
 				GUINT_TO_POINTER(gcd->active_context),
 				conn_compare_by_cid);
 
 	if (!l) {
-		DBG("Did not find data (device and channel id)"
-					"for connection with cid; %d",
-					gcd->active_context);
+		DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
 		goto error;
 	}
 
 	conn = l->data;
 
-	if (!ok)
-		goto error;
+	if (!ok) {
+		conn->cid = 0;
+		gcd->active_context = 0;
+		decode_at_error(&error, g_at_result_final_response(result));
+		cb(&error, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
+		return;
+	}
 
 	rsp.current = NULL;
 	context = g_markup_parse_context_new(&parser, 0, &rsp, NULL);

@@ -266,7 +309,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 
 	for (i = 0; i < g_at_result_num_response_lines(result); i++) {
 		g_at_result_iter_next(&iter, NULL);
-		res_string = strdup(g_at_result_iter_raw_line(&iter));
+		res_string = g_at_result_iter_raw_line(&iter);
 		length = strlen(res_string);
 
 		if (!g_markup_parse_context_parse(context, res_string,

@@ -283,20 +326,9 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	dns[1] = rsp.dns_server2;
 	dns[2] = NULL;
 
-	sprintf(conn->interface, "caif%u", conn->device);
-
-	if (!caif_if_create(conn->interface, conn->channel_id)) {
-		ofono_error("Failed to create caif interface %s.",
-				conn->interface);
-		CALLBACK_WITH_SUCCESS(cb, NULL, FALSE, rsp.ip_address,
+	CALLBACK_WITH_SUCCESS(cb, conn->interface, TRUE, rsp.ip_address,
 				rsp.subnet_mask, rsp.default_gateway,
 				dns, cbd->data);
-	} else {
-		CALLBACK_WITH_SUCCESS(cb, conn->interface,
-				FALSE, rsp.ip_address, rsp.subnet_mask,
-				rsp.default_gateway, dns, cbd->data);
-	}
-
 	return;
 
 error:

@@ -308,6 +340,7 @@ error:
 	if (conn)
 		conn->cid = 0;
 
+	gcd->active_context = 0;
 	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
 }
 

@@ -319,12 +352,23 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 	struct cb_data *ncbd = NULL;
 	char buf[128];
-	struct conn_info *conn;
+	struct conn_info *conn = NULL;
 	GSList *l;
 
+	l = g_slist_find_custom(g_caif_devices,
+				GUINT_TO_POINTER(gcd->active_context),
+				conn_compare_by_cid);
+
+	if (!l) {
+		DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
+		goto error;
+	}
+
+	conn = l->data;
+
 	if (!ok) {
 		struct ofono_error error;
-
+		conn->cid = 0;
 		gcd->active_context = 0;
 
 		decode_at_error(&error, g_at_result_final_response(result));

@@ -332,26 +376,18 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
 		return;
 	}
 
-	ncbd = g_memdup(cbd, sizeof(struct cb_data));
-
-	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(0),
-				conn_compare_by_cid);
-
-	if (!l) {
-		DBG("at_cgdcont_cb, no more available devices");
-		goto error;
-	}
-
-	conn = l->data;
-	conn->cid = gcd->active_context;
 	snprintf(buf, sizeof(buf), "AT*EPPSD=1,%u,%u",
 			conn->channel_id, conn->cid);
+	ncbd = g_memdup(cbd, sizeof(struct cb_data));
 
 	if (g_at_chat_send(gcd->chat, buf, NULL,
 				ste_eppsd_up_cb, ncbd, g_free) > 0)
 		return;
 
 error:
+	if (conn)
+		conn->cid = 0;
+
 	g_free(ncbd);
 
 	gcd->active_context = 0;

@@ -368,6 +404,8 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	struct cb_data *cbd = cb_data_new(cb, data);
 	char buf[AUTH_BUF_LENGTH];
 	int len;
+	GSList *l;
+	struct conn_info *conn = NULL;
 
 	if (!cbd)
 		goto error;

@@ -375,6 +413,23 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	gcd->active_context = ctx->cid;
 	cbd->user = gc;
 
+	/* Find free connection with cid zero */
+	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(0),
+				conn_compare_by_cid);
+
+	if (!l) {
+		DBG("No more available CAIF devices");
+		goto error;
+	}
+
+	conn = l->data;
+	if (!conn->created) {
+		DBG("CAIF interface not created (rtnl error?)");
+		goto error;
+	}
+
+	conn->cid = ctx->cid;
+
 	len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"", ctx->cid);
 
 	if (ctx->apn)

@@ -389,7 +444,7 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	 * Set username and password, this should be done after CGDCONT
 	 * or an error can occur.  We don't bother with error checking
 	 * here
-	 * */
+	 */
 	snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"",
 			ctx->cid, ctx->username, ctx->password);
 

@@ -398,6 +453,10 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	return;
 
 error:
+	if (conn)
+		conn->cid = 0;
+
+	gcd->active_context = 0;
 	g_free(cbd);
 
 	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL, NULL, data);

@@ -423,8 +482,8 @@ static void ste_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 				conn_compare_by_cid);
 
 	if (!l) {
-		DBG("at_gprs_deactivate_primary, did not find"
-			"data (channel id) for connection with cid; %d", id);
+		DBG("did not find data (channel id) "
+				"for connection with cid; %d", id);
 		goto error;
 	}
 

@@ -516,9 +575,11 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 	ofono_gprs_context_set_data(gc, gcd);
 
 	for (i = 0; i < MAX_CAIF_DEVICES; i++) {
-		ci = conn_info_create(i, i+1);
-		if (ci)
-			g_caif_devices = g_slist_append(g_caif_devices, ci);
+		ci = conn_info_create(i+1);
+		if (!ci)
+			return -ENOMEM;
+		caif_if_create(ci);
+		g_caif_devices = g_slist_append(g_caif_devices, ci);
 	}
 
 	return 0;

@@ -527,7 +588,7 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 static void ste_gprs_context_remove(struct ofono_gprs_context *gc)
 {
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-
+	g_slist_foreach(g_caif_devices, (GFunc) caif_if_remove, NULL);
 	g_slist_foreach(g_caif_devices, (GFunc) g_free, NULL);
 	g_slist_free(g_caif_devices);
 	g_caif_devices = NULL;

@@ -548,10 +609,12 @@ static struct ofono_gprs_context_driver driver = {
 
 void ste_gprs_context_init()

Can you please not try to squeeze such change in the same patch. Just have a separate one that does this change.

The more I looked through the rest of this patch, the more I thought you need to split this patch into a cleanup and caif_rtnl usage patch.

So I realized now that you are using ifindex as input for caif_rtnl_delete_interface, now I am questioning why there is a result parameter in the callback for caif_rtnl_create_interface.

Why are we having this in the first place. CAIF network interfaces are point-to-point, right? In that case you should just leave this NULL.

Please use '\0' instead of just 0. You might need to fix this at other places as well.

Quoted 5 lines

drivers/stemodem/gprs-context.c +5 -0

+static void rtnl_callback(int result, gpointer user_data,
+		char *ifname, int ifindex)
+{
+	struct conn_info *conn = user_data;
+

drivers/stemodem/gprs-context.c static struct conn_info *conn_info_create(unsigned int device, · line 193

Quoted 10 lines

drivers/stemodem/gprs-context.c +10 -0

+	strncpy(conn->interface, ifname, sizeof(conn->interface));
+	conn->ifindex = ifindex;
+
+	if (result == 0)
+		conn->created = TRUE;
+	else {
+		conn->created = FALSE;
+		DBG("Could not create CAIF Interface");
+	}
+}

So here is the think. This makes no sense. Either you get an error and ifname and index are not valid or you succeed.

So this should be something like

	if (ifname < 0) {
		connman_error(...)
		return;
	}

I'm sorry about the formatting for the v3 version of this patch. I used git send-email via my gmail account, and ended up with base64 MIME content encoding. I dont' know what went wrong :-(

I think I have closed most of your review comments so far. I kept using sendto as I don't quite get how to use g_io_channel_write_chars for rtnl socket. (I think connman is using sendto as well.) I still set g_caif_devices = NULL just in case someone in future does init/exit more than once.

The patch has been tested activating/deactivation and I have run valgrind showing no leaks on some unit tests (not yet upstreamed).

 Makefile.am                  |    2 +
 drivers/stemodem/caif_rtnl.c |  379 ++++++++++++++++++++++++++++++++++++++++++
 drivers/stemodem/caif_rtnl.h |   29 ++++
 3 files changed, 410 insertions(+), 0 deletions(-)
 create mode 100644 drivers/stemodem/caif_rtnl.c
 create mode 100644 drivers/stemodem/caif_rtnl.h

Makefile.am +2 -0

index 05082de..f163b0a 100644
@@ -226,6 +226,8 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.c \
 			drivers/stemodem/voicecall.c \
 			drivers/stemodem/radio-settings.c \
+			drivers/stemodem/caif_rtnl.c \
+			drivers/stemodem/caif_rtnl.h \
 			drivers/stemodem/gprs-context.c \
 			drivers/stemodem/caif_socket.h \
 			drivers/stemodem/if_caif.h

drivers/stemodem/caif_rtnl.c new file +32 -0

new file mode 100644
index 0000000..ad58c93
@@ -0,0 +1,379 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *
+ *  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
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+#include <net/if.h>
+#include <net/if_arp.h>
+#include <fcntl.h>
+#include <linux/rtnetlink.h>

drivers/stemodem/caif_rtnl.c new file +5 -0

+
+	int request_id;
+	int type;
+	int conn_id;
+	gpointer user_data;

drivers/stemodem/caif_rtnl.c new file +8 -0

+static gboolean get_ifname(struct ifinfomsg *msg, int bytes,
+				const char **ifname)
+{
+	struct rtattr *attr;
+
+	for (attr = IFLA_RTA(msg); RTA_OK(attr, bytes);
+		attr = RTA_NEXT(attr, bytes)) {
+

drivers/stemodem/caif_rtnl.c new file +7 -0

+		DBG("attribute to large for message %d %d %d\n",
+				n->nlmsg_len, len, maxlen);
+		return -1;
+	}
+
+	rta = NLMSG_TAIL(n);
+	rta->rta_type = type;

drivers/stemodem/caif_rtnl.c new file +4 -0

+	struct rtattr *linkinfo;
+	struct rtattr *data;
+
+	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));

drivers/stemodem/caif_rtnl.c new file +30 -0

+	case IFLA_CAIF_IPV4_CONNID:
+		add_attribute(&req->n, sizeof(*req),
+				IFLA_CAIF_IPV4_CONNID, &req->conn_id,
+				sizeof(req->conn_id));
+		break;
+
+	case IFLA_CAIF_IPV6_CONNID:
+		add_attribute(&req->n, sizeof(*req),
+				IFLA_CAIF_IPV6_CONNID, &req->conn_id,
+				sizeof(req->conn_id));
+		break;
+
+	case __IFLA_CAIF_UNSPEC:
+	case IFLA_CAIF_LOOPBACK:
+	case __IFLA_CAIF_MAX:
+		DBG("unsupported linktype");
+		return -EINVAL;
+	}
+
+	if (req->loop_enabled) {
+		int loop = 1;
+		add_attribute(&req->n, sizeof(*req),
+				IFLA_CAIF_LOOPBACK, &loop, sizeof(loop));
+	}
+
+	data->rta_len = (void *)NLMSG_TAIL(&req->n) - (void *)data;
+	linkinfo->rta_len = (void *)NLMSG_TAIL(&req->n) - (void *)linkinfo;
+
+	return 0;
+}

drivers/stemodem/caif_rtnl.c new file +11 -0

+static gboolean netlink_event(GIOChannel *chan,
+				GIOCondition cond, gpointer data)
+{
+	unsigned char buf[4096];
+	gsize len;
+	GIOError err;
+
+	if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR)) {
+		rtnl_watch = 0;
+		return FALSE;
+	}

drivers/stemodem/caif_rtnl.c new file +7 -0

+	parse_rtnl_message(buf, len);
+
+	return TRUE;
+}
+
+int caif_rtnl_init(void)
+{

drivers/stemodem/caif_rtnl.c new file +4 -0

+	err = bind(sk, (struct sockaddr *) &addr, sizeof(addr));
+	if (err < 0) {
+		close(sk);
+		return err;

drivers/stemodem/caif_rtnl.c new file +17 -0

+
+	return 0;
+}
+
+void caif_rtnl_exit(void)
+{
+	GSList *list;
+
+	if (rtnl_watch > 0)
+		g_source_remove(rtnl_watch);
+
+	g_io_channel_unref(channel);
+
+	for (list = pending_requests; list; list = list->next) {
+		struct iplink_req *req = list->data;
+		g_free(req);
+	}

drivers/stemodem/caif_rtnl.c new file +4 -0

+
+	req = g_try_new0(struct iplink_req, 1);
+	if (req == NULL)
+		return -ENOMEM;

drivers/stemodem/caif_rtnl.c new file +9 -0

+error:
+	g_free(req);
+	return err;
+
+}
+
+int caif_rtnl_delete_interface(int ifid)
+{
+	struct iplink_req req;

drivers/stemodem/caif_rtnl.h new file +12 -0

new file mode 100644
index 0000000..8eb1e5f
@@ -0,0 +1,29 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *
+ *  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
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of

what happens for network triggered deactivation. Some networks here disconnect the GPRS context used MMS. Has some funny behavior that need to be taken into account.

Quoted 3 lines

(quoted diff) +2 -0

+#define NLMSG_TAIL(nmsg) \
+   ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-

nlmsg_len)))

We have no global define this? You wanted to look into that. What was the outcome of it?

drivers/stemodem/caif_rtnl.c line 47

Quoted 5 lines

drivers/stemodem/caif_rtnl.c new file +4 -0

+struct iplink_req {
+   struct nlmsghdr n;
+   struct ifinfomsg i;
+   char pad[1024];

What is this huge pad for? It looks totally fishy to me.

Quoted 2 lines

+static guint rtnl_watch;

Get rid of the double spaces for rtnl_watch.

Quoted 1 line

+static GIOChannel *channel;

If we follow your convention here and not to overload variables this might be better named rtnl_channel.

And yes, I am fine with keeping it since there is no way to connect a netlink socket properly to just use send(). You will require to use sendto for everything.

Quoted 2 lines

+static void store_newlink_param(struct iplink_req *req, unsigned

short type,

drivers/stemodem/caif_rtnl.c line 96

Quoted 12 lines

drivers/stemodem/caif_rtnl.c new file +12 -0

+                                   int index, unsigned flags,
+                                   unsigned change, struct ifinfomsg *msg,
+                                   int bytes)
+{
+   const char *ifname = NULL;
+
+   get_ifname(msg, bytes, &ifname);
+   strncpy(req->ifname, ifname,
+                   sizeof(req->ifname));
+   req->ifname[sizeof(req->ifname)-1] = '\0';
+   req->ifindex = index;
+}

So I think that store_newlink_... and get_ifname can be nicely combined into one helper function. And using extract_parameters() function name is a bit better.

And please only add parameters to that function that you really need in there. And the iplink_req should be last parameter.

As one general is to have input parameters first and then the output parameters last. So read this something like extract values from this structure to this structure.

drivers/stemodem/caif_rtnl.c line 108

Quoted 12 lines

drivers/stemodem/caif_rtnl.c new file +11 -0

+static int send_rtnl_req(struct iplink_req *req)
+{
+   struct sockaddr_nl addr;
+   int sk = g_io_channel_unix_get_fd(channel);
+
+   memset(&addr, 0, sizeof(addr));
+   addr.nl_family = AF_NETLINK;
+
+   return sendto(sk, req, req->n.nlmsg_len, 0,
+                   (struct sockaddr *) &addr, sizeof(addr));
+}

I don't think this should be a separate function. You use it only twice and having it close to the code using it would be better.

Quoted 4 lines

drivers/stemodem/caif_rtnl.c new file +3 -0

+static struct iplink_req *find_request(guint32 seq)
+{
+   GSList *list;

drivers/stemodem/caif_rtnl.c line 122

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+
+   for (list = pending_requests; list; list = list->next) {
+           struct iplink_req *req = list->data;
+
+           if (req->n.nlmsg_seq == seq)
+                   return req;
+   }
+
+   return NULL;
+}

I would move this function up in this file at the top. It should go close to the variable declaration for pending_request.

drivers/stemodem/caif_rtnl.c line 133

Quoted 11 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+static void rtnl_client_response(struct iplink_req *req, int result)
+{
+
+   if (req->callback && req->n.nlmsg_type == RTM_NEWLINK)
+           req->callback(result, req->user_data,
+                           req->ifname, req->ifindex);
+
+   pending_requests = g_slist_remove(pending_requests, req);
+   g_free(req);
+}

I don't like this split out. You already checked the nlmsg_type and here
you keep checking it again just to reuse some code. This looks like waste to me.

drivers/stemodem/caif_rtnl.c line 138

Quoted 5 lines

drivers/stemodem/caif_rtnl.c new file +4 -0

+static void parse_rtnl_message(void *buf, size_t len)
+{
+   struct ifinfomsg *msg;
+   struct iplink_req *req;

Make it const void *buf.

An you can have more than one RTNL message inside this buffer. You need to be able to handle this. Otherwise you might loose responses. The case
of just calling return when you received one of the two messages you care about is not really acceptable.

drivers/stemodem/caif_rtnl.c line 149

Quoted 11 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+   while (len > 0) {
+           struct nlmsghdr *hdr = buf;
+
+           if (!NLMSG_OK(hdr, len))
+                   break;
+
+           if (hdr->nlmsg_type == RTM_NEWLINK) {
+                   req = g_slist_nth_data(pending_requests, 0);
+                   if (req == NULL)
+                           break;

How does this work? You just pick the first pending request and don't really compare the sequence number. Who guarantees the order of these events. I don't think we should be doing this.

drivers/stemodem/caif_rtnl.c line 159

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +9 -0

+                   msg = (struct ifinfomsg *) NLMSG_DATA(hdr);
+                   store_newlink_param(req, msg->ifi_type,
+                                   msg->ifi_index, msg->ifi_flags,
+                                   msg->ifi_change, msg,
+                                   IFA_PAYLOAD(hdr));
+                   rtnl_client_response(req, 0);
+                   return;
+
+           } else if (hdr->nlmsg_type == NLMSG_ERROR) {

So I would prefer if you use a switch statement here. It is just easier to read. Especially inside a while() loop.

drivers/stemodem/caif_rtnl.c line 167

Quoted 9 lines

drivers/stemodem/caif_rtnl.c new file +8 -0

+                   req = find_request(hdr->nlmsg_seq);
+                   if (req == NULL)
+                           break;
+
+                   DBG("nlmsg error req");
+                   rtnl_client_response(req, -1);
+                   return;
+           }

If I understand this right, then you can always find the pending request
and remove it here. No need for rtnl_client_response at all.

drivers/stemodem/caif_rtnl.c line 180

Quoted 13 lines

drivers/stemodem/caif_rtnl.c new file +12 -0

+           len -= hdr->nlmsg_len;
+           buf += hdr->nlmsg_len;
+   }
+}
+
+static int add_attribute(struct nlmsghdr *n, int maxlen, int type,
+                           const void *data, int datalen)
+{
+   int len = RTA_LENGTH(datalen);
+   struct rtattr *rta;
+
+   if ((int)(NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len)) > maxlen) {

Why is this (int) cast needed. Can we just not used proper parameter for
maxlen in the first place?

drivers/stemodem/caif_rtnl.c line 211

Quoted 3 lines

drivers/stemodem/caif_rtnl.c new file +2 -0

+   add_attribute(&req->n, sizeof(*req), IFLA_INFO_KIND,
+                   type, strlen(type));

So add_attribute(..., "caif", 4) should be just fine.

Quoted 2 lines

+ data = NLMSG_TAIL(&req->n);

Why is this assignment here and only used later on. I see that you do it
for length calculation, but use a common variable name like data is a bit confusing.

+static void prep_rtnl_dellink_req(struct iplink_req *req, int ifindex)

drivers/stemodem/caif_rtnl.c line 257

Quoted 8 lines

drivers/stemodem/caif_rtnl.c new file +8 -0

+{
+   req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
+   req->n.nlmsg_flags = NLM_F_REQUEST|NLM_F_CREATE|NLM_F_EXCL;
+   req->n.nlmsg_type = RTM_DELLINK;
+   req->n.nlmsg_seq = ++rtnl_seqnr;
+   req->i.ifi_family = AF_UNSPEC;
+   req->i.ifi_index = ifindex;
+}

Actually having some generic function with a parameter of nlmsg_type that fills these details would be a good thing.

Quoted 12 lines

drivers/stemodem/caif_rtnl.c new file +11 -0

+static gboolean netlink_event(GIOChannel *chan,
+                           GIOCondition cond, gpointer data)
+{
+   unsigned char buf[4096];
+   gsize len;
+   GIOError err;
+
+   if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR)) {
+           rtnl_watch = 0;
+           return FALSE;
+   }

drivers/stemodem/caif_rtnl.c line 279

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+
+   memset(buf, 0, sizeof(buf));
+
+   err = g_io_channel_read(chan, (gchar *) buf, sizeof(buf), &len);
+   if (err) {
+           if (err == G_IO_ERROR_AGAIN)
+                   return TRUE;
+           rtnl_watch = 0;
+           return FALSE;
+   }

Just use recv() here. I know that I would prefer to be using GIOChannel functions, but they don't work well on netlink sockets anyway, so don't bother with them at all.

Sorry for confusing you and sending you into the wrong direction first.

Quoted 8 lines

drivers/stemodem/caif_rtnl.c new file +7 -0

+   parse_rtnl_message(buf, len);
+
+   return TRUE;
+}
+
+int caif_rtnl_init(void)
+{

drivers/stemodem/caif_rtnl.c

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+   struct sockaddr_nl addr;
+   int sk, err;
+
+   sk = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
+   if (sk < 0)
+           return sk;
+
+   memset(&addr, 0, sizeof(addr));
+   addr.nl_family = AF_NETLINK;
+   addr.nl_groups = RTMGRP_LINK | RTMGRP_IPV4_IFADDR |

RTMGRP_IPV4_ROUTE;

You need IPV4_IFADDR and IPV4_ROUTE as well. I don't see you making use of them. Please only set LING since we don't wanna wake ofonod up for anything else.

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +9 -0

+   err = bind(sk, (struct sockaddr *) &addr, sizeof(addr));
+   if (err < 0) {
+           close(sk);
+           return err;
+   }
+
+   channel = g_io_channel_unix_new(sk);
+   g_io_channel_set_flags(channel, G_IO_FLAG_NONBLOCK, NULL);
+   g_io_channel_set_close_on_unref(channel, TRUE);

drivers/stemodem/caif_rtnl.c line 312

Quoted 5 lines

drivers/stemodem/caif_rtnl.c new file +5 -0

+
+   rtnl_watch = g_io_add_watch(channel,
+                           G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
+                           netlink_event, NULL);
+   pending_requests = NULL;

Please don't bother with setting it here to zero. Just set the global variable to NULL. The init and exit will be only called once anyway.

Quoted 14 lines

drivers/stemodem/caif_rtnl.c new file +13 -0

+
+   return 0;
+}
+
+void caif_rtnl_exit(void)
+{
+   GSList *list;
+
+   if (rtnl_watch > 0)
+           g_source_remove(rtnl_watch);
+
+   g_io_channel_unref(channel);
+

drivers/stemodem/caif_rtnl.c

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+   for (list = pending_requests; list; list = list->next) {
+           struct iplink_req *req = list->data;
+           g_free(req);
+   }
+
+   g_slist_free(pending_requests);
+}
+
+
+int caif_rtnl_create_interface(gpointer user_data, int type, int

connid,

drivers/stemodem/caif_rtnl.c line 339

Quoted 4 lines

drivers/stemodem/caif_rtnl.c new file +4 -0

+           gboolean loop, caif_rtnl_create_cb_t cb)
+{
+   int err;
+   struct iplink_req *req;

I prefer structs before int. Just a minor style thing.

Quoted 11 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+
+   req = g_try_new0(struct iplink_req, 1);
+   if (req == NULL)
+           return -ENOMEM;
+
+   req->user_data = user_data;
+   req->type = type;
+   req->conn_id = connid;
+   req->loop_enabled = loop;
+   req->callback = cb;

drivers/stemodem/caif_rtnl.c line 353

Quoted 4 lines

drivers/stemodem/caif_rtnl.c new file +4 -0

+
+   err = prep_rtnl_newlink_req(req);
+   if (err < 0)
+           goto error;

So if we split out creating the NLMSG header into one helper functions, then you can easily prep the RTNL newlink message here inside the function.

This will also allow you to remove req->loop_enabled variables since I don't see need for storing them.

drivers/stemodem/caif_rtnl.c line 358

Quoted 6 lines

drivers/stemodem/caif_rtnl.c new file +5 -0

+
+   err = send_rtnl_req(req);
+   if (err < 0)
+           goto error;
+

Same here. Just putting the sendto() directly in here seems just fine.

Review

Quoted 3 lines

(quoted diff) +2 -0

+   pending_requests = g_slist_append(pending_requests, req);
+   return 0;

Extra empty line before a label please.

Quoted 13 lines

drivers/stemodem/caif_rtnl.c new file +13 -0

+error:
+   g_free(req);
+   return err;
+
+}
+
+int caif_rtnl_delete_interface(int ifid)
+{
+   struct iplink_req req;
+   int err;
+   memset(&req, 0, sizeof(req));
+   prep_rtnl_dellink_req(&req, ifid);
+

drivers/stemodem/caif_rtnl.c line 379

Quoted 6 lines

drivers/stemodem/caif_rtnl.c new file +6 -0

+   err = send_rtnl_req(&req);
+   if (err < 0)
+           return err;
+
+   return 0;
+}

Comments from above apply here.

Quoted 2 lines

(quoted diff) +0 -0

Quoted 1 line

+typedef void (*caif_rtnl_create_cb_t) (int result, gpointer

user_data,

Quoted 1 line

+ char *ifname, int ifindex);

Please don't use GLib types here. Using int and void * is better.

So the user_data parameter should come always last. Do we need index and
ifname? I think inside oFono we only care about ifname.

Quoted 2 lines

+extern int caif_rtnl_create_interface(gpointer user_data, int type,

int connid,

Quoted 1 line

+ gboolean loop, caif_rtnl_create_cb_t cb);

The user_data is always the last parameter. The only exception would be having a destruct callback.

Please use int for loop.

Quoted 3 lines

what happens for network triggered deactivation. Some networks here disconnect the GPRS context used MMS. Has some funny behavior that need to be taken into account.

Good question! When code reading with this in mind I realize that we missed to mark the conn_info->interface as available by setting cid = 0. This is bug, thank you for leading me to it.

FYI this is how CAIF and AT works together:
In order to have payload over CAIF there need to be
both a CAIF channel connected and a context activated with AT*EPPSD. When Channel-ID configured for CAIF IP Interface and the Channel-Id given in AT*EPPSD matches, the modem side will connect the CAIF Channel to the Network Signaling Stack in the modem.

With this new patch CAIF channels are created statically from probe. Activation and deactivation is controlled by AT*EPPSD, or notified by +CGEV which will result in a CGACT status query.

If the network disconnects GPRS we should receive a "+CGEV: NW DEACT", subsequent CGACT status query. Here the connection (and CAIF Network Interface) should be marked as unused by setting cid to zero.

Quoted 6 lines

Quoted 3 lines

(quoted diff) +2 -0

+#define NLMSG_TAIL(nmsg) \
+   ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-

nlmsg_len)))

We have no global define this? You wanted to look into that. What was the outcome of it?

I did mention this before - I have looked around, but he did not find any relevant macro for this unfortunately.

Quoted 6 lines

drivers/stemodem/caif_rtnl.c line 47

Quoted 4 lines

drivers/stemodem/caif_rtnl.c new file +4 -0

+struct iplink_req {
+   struct nlmsghdr n;
+   struct ifinfomsg i;
+   char pad[1024];

What is this huge pad for? It looks totally fishy to me.

The pad is there to hold the remaining of the rtnl attributes. I'm working on restructuring this so that I can separate the buffer holding the rtnl_message and the iplink_req. I just have to make it work...

Quoted 7 lines

Quoted 7 lines

drivers/stemodem/caif_rtnl.c new file +7 -0

+static void rtnl_client_response(struct iplink_req *req, int result)
+{
+
+   if (req->callback && req->n.nlmsg_type == RTM_NEWLINK)
+           req->callback(result, req->user_data,
+                           req->ifname, req->ifindex);
+

Quoted 8 lines

drivers/stemodem/caif_rtnl.c line 133

Quoted 3 lines

drivers/stemodem/caif_rtnl.c new file +3 -0

+   pending_requests = g_slist_remove(pending_requests, req);
+   g_free(req);
+}

I don't like this split out. You already checked the nlmsg_type and here
you keep checking it again just to reuse some code. This looks like waste to me.

I have squashed this into parse_rtnl_message, as you suggested. I'm not really convinced this was any cleaner though..? I end up with duplicating the code in the two if statements.

Quoted 5 lines

An you can have more than one RTNL message inside this buffer. You need to be able to handle this. Otherwise you might loose responses. The case
of just calling return when you received one of the two messages you care about is not really acceptable.

OK, done. I'll keep looping.

Quoted 15 lines

drivers/stemodem/caif_rtnl.c line 149

Quoted 11 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+   while (len > 0) {
+           struct nlmsghdr *hdr = buf;
+
+           if (!NLMSG_OK(hdr, len))
+                   break;
+
+           if (hdr->nlmsg_type == RTM_NEWLINK) {
+                   req = g_slist_nth_data(pending_requests, 0);
+                   if (req == NULL)
+                           break;

How does this work? You just pick the first pending request and don't really compare the sequence number. Who guarantees the order of these events. I don't think we should be doing this.

I think that the kernel implementation of rtnl will guarantee the requests to be processed in order. rtnetlink_rcv in ..net/core/rtnetlink.c takes the rtnl_lock before processing the netlink messages.
I believe this will guarantee that the NEWLINK responses will be received in the same order as they are sent.

Quoted 10 lines

Quoted 9 lines

drivers/stemodem/caif_rtnl.c new file +9 -0

+                   msg = (struct ifinfomsg *) NLMSG_DATA(hdr);
+                   store_newlink_param(req, msg->ifi_type,
+                                   msg->ifi_index, msg->ifi_flags,
+                                   msg->ifi_change, msg,
+                                   IFA_PAYLOAD(hdr));
+                   rtnl_client_response(req, 0);
+                   return;
+
+           } else if (hdr->nlmsg_type == NLMSG_ERROR) {

Quoted 2 lines

So I would prefer if you use a switch statement here. It is just easier to read. Especially inside a while() loop.

OK - I'll break coding-style rule M12. There are more than 40 RTM messages I'm not handling, but no rule without exception :-)

Quoted 9 lines

Quoted 8 lines

drivers/stemodem/caif_rtnl.c new file +8 -0

+                   req = find_request(hdr->nlmsg_seq);
+                   if (req == NULL)
+                           break;
+
+                   DBG("nlmsg error req");
+                   rtnl_client_response(req, -1);
+                   return;
+           }

Quoted 3 lines

If I understand this right, then you can always find the pending request
and remove it here. No need for rtnl_client_response at all.

Yes, I guess you are right. I can remove this callback. Currently I am only actually interested in the result when the interface is created successfully. Nothing particularly is done in gprs-context.c if it fails.

Quoted 26 lines

Quoted 11 lines

drivers/stemodem/caif_rtnl.c new file +11 -0

+static gboolean netlink_event(GIOChannel *chan,
+                           GIOCondition cond, gpointer data)
+{
+   unsigned char buf[4096];
+   gsize len;
+   GIOError err;
+
+   if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR)) {
+           rtnl_watch = 0;
+           return FALSE;
+   }

drivers/stemodem/caif_rtnl.c line 279

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+
+   memset(buf, 0, sizeof(buf));
+
+   err = g_io_channel_read(chan, (gchar *) buf, sizeof(buf), &len);
+   if (err) {
+           if (err == G_IO_ERROR_AGAIN)
+                   return TRUE;
+           rtnl_watch = 0;
+           return FALSE;
+   }

Just use recv() here. I know that I would prefer to be using GIOChannel functions, but they don't work well on netlink sockets anyway, so don't bother with them at all.

Quoted 1 line

Sorry for confusing you and sending you into the wrong direction first.

Actually I think io_channel_read is just fine here. I think connman is doing the same thing for rtnl. It is the sending part which is problematic.

Quoted 19 lines

Quoted 7 lines

drivers/stemodem/caif_rtnl.c new file +7 -0

+   parse_rtnl_message(buf, len);
+
+   return TRUE;
+}
+
+int caif_rtnl_init(void)
+{

drivers/stemodem/caif_rtnl.c

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+   struct sockaddr_nl addr;
+   int sk, err;
+
+   sk = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
+   if (sk < 0)
+           return sk;
+
+   memset(&addr, 0, sizeof(addr));
+   addr.nl_family = AF_NETLINK;
+   addr.nl_groups = RTMGRP_LINK | RTMGRP_IPV4_IFADDR |

RTMGRP_IPV4_ROUTE;

Quoted 3 lines

You need IPV4_IFADDR and IPV4_ROUTE as well. I don't see you making use of them. Please only set LING since we don't wanna wake ofonod up for anything else.

Actually I guess I only need RTMGRP_LINK. I am only interested in creating and removing the CAIF Interface. Setting the IP addresses and all the rest is connman responsibility right?

Quoted 10 lines

Quoted 10 lines

drivers/stemodem/caif_rtnl.c new file +10 -0

+
+   req = g_try_new0(struct iplink_req, 1);
+   if (req == NULL)
+           return -ENOMEM;
+
+   req->user_data = user_data;
+   req->type = type;
+   req->conn_id = connid;
+   req->loop_enabled = loop;
+   req->callback = cb;

Quoted 8 lines

drivers/stemodem/caif_rtnl.c line 353

Quoted 4 lines

drivers/stemodem/caif_rtnl.c new file +4 -0

+
+   err = prep_rtnl_newlink_req(req);
+   if (err < 0)
+           goto error;

So if we split out creating the NLMSG header into one helper functions, then you can easily prep the RTNL newlink message here inside the function.

Even with creating this helper function you suggested
the prep_rtnl_newlink_req is on 45 lines. I think it makes sense still keep then separate. It is no big deal for me, but I would prefer to avoid such a large functions,

Quoted 2 lines

This will also allow you to remove req->loop_enabled variables since I don't see need for storing them.

You are right, the loop_enabled variable is not currently needed. However for various testing purposes we frequently want to run the CAIF Interface in loopback mode (e.g. testing loopback to the modem). loopback_mode will make the interface swap src and destination ip addresses so we actually can do ping etc. If you are OK with this, I would prefer to keep this in order to make testing easier. But again, I'm easy...

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -0

...

Quoted 6 lines

Quoted 1 line

+typedef void (*caif_rtnl_create_cb_t) (int result, gpointer

user_data,

Quoted 1 line

+ char *ifname, int ifindex);

Please don't use GLib types here. Using int and void * is better.

Quoted 3 lines

So the user_data parameter should come always last. Do we need index and
ifname? I think inside oFono we only care about ifname.

Yes, but ifindex is used when removing the CAIF IP Interface. It makes life a lot easier when removing an interface.

Thank you for a very thorough review here Marcel.

I have done most of the updates, I could sent what I have now, but I'd like to make it work again. Removing the pad and split apart the request structure and the actual rtnl message proved to be a bit of work....

Does anything cleans up the channel when oFono unexpectedly crashes?

I think you can just use a stack buffer when you actually send the message. This is damn ugly and from a security point it actually scares me.

Please only keep the seqnum in here. Since that is the only thing you need to find that pending request. Just get rid of everything that you don't need for the callback.

Quoted 3 lines

I have squashed this into parse_rtnl_message, as you suggested. I'm not really convinced this was any cleaner though..? I end up with duplicating the code in the two if statements.

We will see. I might have been wrong, but in my mind it just locked fine. I will take a second look when you send the updated patch.

Quoted 2 lines

I think that the kernel implementation of rtnl will guarantee the requests to be processed in order. rtnetlink_rcv in

..net/core/rtnetlink.c

Quoted 2 lines

takes the rtnl_lock before processing the netlink messages. I believe this will guarantee that the NEWLINK responses will be

received

Quoted 1 line

in the same order as they are sent.

it might be keeping the order, but I wouldn't count on it. And just using your find request function would make this code simpler.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

drivers/stemodem/caif_rtnl.c new file +8 -0

+                   req = find_request(hdr->nlmsg_seq);
+                   if (req == NULL)
+                           break;
+
+                   DBG("nlmsg error req");
+                   rtnl_client_response(req, -1);
+                   return;
+           }

Quoted 8 lines

Quoted 3 lines

If I understand this right, then you can always find the pending request
and remove it here. No need for rtnl_client_response at all.

Yes, I guess you are right. I can remove this callback. Currently I am only actually interested in the result when the interface is created successfully. Nothing particularly is done in gprs-context.c if it fails.

That is not what I meant. We should find that request and call the callback, but I think this can be done a more generic way.

So whatever message you get, find the request that it belongs to. You need that request struct anyway. And then just reply here.

Quoted 1 line

Actually I think io_channel_read is just fine here. I think connman

is doing

Quoted 1 line

the same thing for rtnl. It is the sending part which is problematic.

We should not be using that in ConnMan either actually. The _read is fine, but deprecated. The new _read_chars is doing buffering and a bit nasty in that area. So really just use recv() here.

Quoted 1 line

Quoted 1 line

So if we split out creating the NLMSG header into one helper

functions,

Quoted 6 lines

Quoted 2 lines

then you can easily prep the RTNL newlink message here inside the function.

Even with creating this helper function you suggested the prep_rtnl_newlink_req is on 45 lines. I think it makes sense still keep then separate. It is no big deal for me, but I would

prefer

Quoted 1 line

to avoid such a large functions,

As I said above, it looked good in my head. Maybe I was wrong, but in general this is pretty simple sequential stuff. So it can be in one function and still be easy readable. Lets have a look on how it looks and then decide.

Quoted 2 lines

Quoted 2 lines

This will also allow you to remove req->loop_enabled variables

since I

Quoted 4 lines

Quoted 1 line

don't see need for storing them.

You are right, the loop_enabled variable is not currently needed. However for various testing purposes we frequently want to run the

CAIF Interface

Quoted 1 line

in loopback mode (e.g. testing loopback to the modem). loopback_mode

will make the

Quoted 1 line

interface swap src and destination ip addresses so we actually can do

ping etc.

Quoted 1 line

If you are OK with this, I would prefer to keep this in order to make

testing easier.

Quoted 1 line

But again, I'm easy...

Having loopback mode is fine. I just meant that you don't have to store the variable in the request struct since you build the request already.

Please store only things in your request struct that you do need later on.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +0 -0

...

Quoted 3 lines

Quoted 1 line

+typedef void (*caif_rtnl_create_cb_t) (int result, gpointer

user_data,

Quoted 1 line

+ char *ifname, int ifindex);

Quoted 2 lines

Yes, but ifindex is used when removing the CAIF IP Interface. It makes life a lot easier when removing an interface.

I saw that later. Then the result is pointless. Just negative ifindex could be used for errors.

So this should be (int index, const char *interface, void *user_data).

Quoted 1 line

Thank you for a very thorough review here Marcel.

No problem. Just RTNL takes always a bit to get used to again ;)