[PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

[PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

[-- Attachment #1: Type: text/plain, Size: 12010 bytes --]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

---

Hi Marcel.

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).

Regards,
Sjur

 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

diff --git a/Makefile.am b/Makefile.am
index 05082de..f163b0a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -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
diff --git a/drivers/stemodem/caif_rtnl.c b/drivers/stemodem/caif_rtnl.c
new file mode 100644
index 0000000..ad58c93
--- /dev/null
+++ b/drivers/stemodem/caif_rtnl.c
@@ -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>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+
+#include "if_caif.h"
+#include "caif_rtnl.h"
+
+#define NLMSG_TAIL(nmsg) \
+	((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len)))
+
+struct iplink_req {
+	struct nlmsghdr n;
+	struct ifinfomsg i;
+	char pad[1024];
+
+	int request_id;
+	int type;
+	int conn_id;
+	gpointer user_data;
+	gboolean loop_enabled;
+
+	char ifname[IF_NAMESIZE];
+	int  ifindex;
+	caif_rtnl_create_cb_t callback;
+};
+
+static GSList *pending_requests;
+static guint32 rtnl_seqnr;
+static guint  rtnl_watch;
+static GIOChannel *channel;
+
+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)) {
+
+		if (attr->rta_type == IFLA_IFNAME &&
+				ifname != NULL) {
+
+			*ifname = RTA_DATA(attr);
+			return TRUE;
+		}
+	}
+
+	return FALSE;
+}
+
+static void store_newlink_param(struct iplink_req *req, unsigned short type,
+					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;
+}
+
+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));
+}
+
+static struct iplink_req *find_request(guint32 seq)
+{
+	GSList *list;
+
+	for (list = pending_requests; list; list = list->next) {
+		struct iplink_req *req = list->data;
+
+		if (req->n.nlmsg_seq == seq)
+			return req;
+	}
+
+	return NULL;
+}
+
+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);
+}
+
+static void parse_rtnl_message(void *buf, size_t len)
+{
+	struct ifinfomsg *msg;
+	struct iplink_req *req;
+
+	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;
+
+			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) {
+			req = find_request(hdr->nlmsg_seq);
+			if (req == NULL)
+				break;
+
+			DBG("nlmsg error req");
+			rtnl_client_response(req, -1);
+			return;
+		}
+
+		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) {
+		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;
+	rta->rta_len = len;
+	memcpy(RTA_DATA(rta), data, datalen);
+	n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len);
+
+	return 0;
+}
+
+static int prep_rtnl_newlink_req(struct iplink_req *req)
+{
+	char type[] = "caif";
+	struct rtattr *linkinfo;
+	struct rtattr *data;
+
+	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_NEWLINK;
+	req->n.nlmsg_seq = ++rtnl_seqnr;
+	req->i.ifi_family = AF_UNSPEC;
+
+	linkinfo = NLMSG_TAIL(&req->n);
+	add_attribute(&req->n, sizeof(*req), IFLA_LINKINFO,
+			NULL, 0);
+	add_attribute(&req->n, sizeof(*req), IFLA_INFO_KIND,
+			type, strlen(type));
+	data = NLMSG_TAIL(&req->n);
+	add_attribute(&req->n, sizeof(*req), IFLA_INFO_DATA,
+			NULL, 0);
+
+	switch (req->type) {
+
+	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;
+}
+
+static void prep_rtnl_dellink_req(struct iplink_req *req, int ifindex)
+{
+	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;
+}
+
+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;
+	}
+
+	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;
+	}
+
+	parse_rtnl_message(buf, len);
+
+	return TRUE;
+}
+
+int caif_rtnl_init(void)
+{
+	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;
+
+	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);
+
+	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;
+
+	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);
+	}
+
+	g_slist_free(pending_requests);
+}
+
+
+int caif_rtnl_create_interface(gpointer user_data, int type, int connid,
+		gboolean loop, caif_rtnl_create_cb_t cb)
+{
+	int err;
+	struct iplink_req *req;
+
+	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;
+
+	err = prep_rtnl_newlink_req(req);
+	if (err < 0)
+		goto error;
+
+	err = send_rtnl_req(req);
+	if (err < 0)
+		goto error;
+
+	pending_requests = g_slist_append(pending_requests, req);
+	return 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);
+
+	err = send_rtnl_req(&req);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
diff --git a/drivers/stemodem/caif_rtnl.h b/drivers/stemodem/caif_rtnl.h
new file mode 100644
index 0000000..8eb1e5f
--- /dev/null
+++ b/drivers/stemodem/caif_rtnl.h
@@ -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
+ *  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
+ *
+ */
+typedef void (*caif_rtnl_create_cb_t) (int result, gpointer user_data,
+		char *ifname, int ifindex);
+
+extern int caif_rtnl_create_interface(gpointer user_data, int type, int connid,
+		gboolean loop, caif_rtnl_create_cb_t cb);
+extern int caif_rtnl_delete_interface(int ifid);
+
+extern int caif_rtnl_init(void);
+extern void caif_rtnl_exit(void);
-- 
1.6.3.3

[PATCH v4 2/2] stemodem: Update gprs-context to use rtnl to create/remove interfaces.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

[-- Attachment #1: Type: text/plain, Size: 12037 bytes --]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

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

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index 9f59579..4260d31 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -28,6 +28,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <errno.h>
 
 #include <glib.h>
 
@@ -46,10 +47,11 @@
 #include "stemodem.h"
 #include "caif_socket.h"
 #include "if_caif.h"
+#include "caif_rtnl.h"
 
-#define MAX_CAIF_DEVICES 7
+#define MAX_CAIF_DEVICES 4
 #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 {
+	/*
+	 * cid is allocated in oFono Core and is identifying
+	 * the Account. cid = 0 indicates that it is currently unused.
+	 */
 	unsigned int cid;
-	unsigned int device;
+	/* Id used by CAIF and EPPSD to identify the CAIF channel*/
 	unsigned int channel_id;
-	char interface[10];
+	/* Linux Interface Id */
+	unsigned int ifindex;
+	/* Linux Interface name */
+	char interface[IF_NAMESIZE];
+	gboolean created;
 };
 
 struct eppsd_response {
 	char *current;
-	char ip_address[MAX_ELEM];
-	char subnet_mask[MAX_ELEM];
-	char mtu[MAX_ELEM];
-	char default_gateway[MAX_ELEM];
-	char dns_server1[MAX_ELEM];
-	char dns_server2[MAX_ELEM];
-	char p_cscf_server[MAX_ELEM];
+	char ip_address[IP_ADDR_LEN];
+	char subnet_mask[IP_ADDR_LEN];
+	char mtu[IP_ADDR_LEN];
+	char default_gateway[IP_ADDR_LEN];
+	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,
@@ -122,8 +132,8 @@ static void text_handler(GMarkupParseContext *context,
 	struct eppsd_response *rsp = user_data;
 
 	if (rsp->current) {
-		strncpy(rsp->current, text, MAX_ELEM);
-		rsp->current[MAX_ELEM] = 0;
+		strncpy(rsp->current, text, IP_ADDR_LEN);
+		rsp->current[IP_ADDR_LEN] = 0;
 	}
 }
 
@@ -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;
+
+	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");
+	}
+}
+
 /*
  * 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()
 {
+	caif_rtnl_init();
 	ofono_gprs_context_driver_register(&driver);
 }
 
 void ste_gprs_context_exit()
 {
+	caif_rtnl_exit();
 	ofono_gprs_context_driver_unregister(&driver);
 }
-- 
1.6.3.3

Re: [PATCH v4 2/2] stemodem: Update gprs-context to use rtnl to create/remove interfaces.

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 13770 bytes --]

Hi Sjur,

>  drivers/stemodem/gprs-context.c |  207 +++++++++++++++++++++++++--------------
>  1 files changed, 135 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
> index 9f59579..4260d31 100644
> --- a/drivers/stemodem/gprs-context.c
> +++ b/drivers/stemodem/gprs-context.c
> @@ -28,6 +28,7 @@
>  #include <string.h>
>  #include <stdlib.h>
>  #include <stdio.h>
> +#include <errno.h>
>  
>  #include <glib.h>
>  
> @@ -46,10 +47,11 @@
>  #include "stemodem.h"
>  #include "caif_socket.h"
>  #include "if_caif.h"
> +#include "caif_rtnl.h"
>  
> -#define MAX_CAIF_DEVICES 7
> +#define MAX_CAIF_DEVICES 4

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

>  #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 {
> +	/*
> +	 * cid is allocated in oFono Core and is identifying
> +	 * the Account. cid = 0 indicates that it is currently unused.
> +	 */
>  	unsigned int cid;
> -	unsigned int device;
> +	/* Id used by CAIF and EPPSD to identify the CAIF channel*/
>  	unsigned int channel_id;
> -	char interface[10];
> +	/* Linux Interface Id */
> +	unsigned int ifindex;

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.

> +	/* Linux Interface name */
> +	char interface[IF_NAMESIZE];
> +	gboolean created;
>  };
>  
>  struct eppsd_response {
>  	char *current;
> -	char ip_address[MAX_ELEM];
> -	char subnet_mask[MAX_ELEM];
> -	char mtu[MAX_ELEM];
> -	char default_gateway[MAX_ELEM];
> -	char dns_server1[MAX_ELEM];
> -	char dns_server2[MAX_ELEM];
> -	char p_cscf_server[MAX_ELEM];
> +	char ip_address[IP_ADDR_LEN];
> +	char subnet_mask[IP_ADDR_LEN];
> +	char mtu[IP_ADDR_LEN];
> +	char default_gateway[IP_ADDR_LEN];

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.

> +	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,
> @@ -122,8 +132,8 @@ static void text_handler(GMarkupParseContext *context,
>  	struct eppsd_response *rsp = user_data;
>  
>  	if (rsp->current) {
> -		strncpy(rsp->current, text, MAX_ELEM);
> -		rsp->current[MAX_ELEM] = 0;
> +		strncpy(rsp->current, text, IP_ADDR_LEN);
> +		rsp->current[IP_ADDR_LEN] = 0;

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

>  	}
>  }
>  
> @@ -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;
> +
> +	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;
	}

	....

> +
>  /*
>   * 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()
>  {
> +	caif_rtnl_init();
>  	ofono_gprs_context_driver_register(&driver);
>  }
>  
>  void ste_gprs_context_exit()
>  {
> +	caif_rtnl_exit();
>  	ofono_gprs_context_driver_unregister(&driver);
>  }

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.

Regards

Marcel

RE: [PATCH v4 2/2] stemodem: Update gprs-context to use rtnl to create/remove interfaces.

[Sjur BRENDELAND]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

Hi Marcel.
 
> 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.

Yes, I have broken this into three patches.
It's a bit hard to find the exactly what to put where, and
tear apart what is already squashed. But have a look and
see what you think.

> 
> 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.

OK, agree - done.

> 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.

Yes - agree. PtP link should not report GW.

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

OK, done at least here..

> > +static void rtnl_callback(int result, gpointer user_data,
> > +		char *ifname, int ifindex)
> > +{
> > +	struct conn_info *conn = user_data;
> > +
> > +	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;
> 	}
OK, I've done something similar.


Regards,
Sjur

Re: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 17038 bytes --]

Hi Sjur,

> 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).

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.

>  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
> 
> diff --git a/Makefile.am b/Makefile.am
> index 05082de..f163b0a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -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
> diff --git a/drivers/stemodem/caif_rtnl.c b/drivers/stemodem/caif_rtnl.c
> new file mode 100644
> index 0000000..ad58c93
> --- /dev/null
> +++ b/drivers/stemodem/caif_rtnl.c
> @@ -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>
> +
> +#include <glib.h>
> +
> +#include <ofono/log.h>
> +
> +#include "if_caif.h"
> +#include "caif_rtnl.h"
> +
> +#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?

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

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

> +
> +	int request_id;
> +	int type;
> +	int conn_id;
> +	gpointer user_data;
> +	gboolean loop_enabled;
> +
> +	char ifname[IF_NAMESIZE];
> +	int  ifindex;
> +	caif_rtnl_create_cb_t callback;
> +};
> +
> +static GSList *pending_requests;
> +static guint32 rtnl_seqnr;
> +static guint  rtnl_watch;

Get rid of the double spaces for rtnl_watch.

> +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.

> +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)) {
> +
> +		if (attr->rta_type == IFLA_IFNAME &&
> +				ifname != NULL) {
> +
> +			*ifname = RTA_DATA(attr);
> +			return TRUE;
> +		}
> +	}
> +
> +	return FALSE;
> +}

Any reason why not just return const char * and take NULL as FALSE?

> +static void store_newlink_param(struct iplink_req *req, unsigned short type,
> +					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.

> +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.

> +static struct iplink_req *find_request(guint32 seq)
> +{
> +	GSList *list;
> +
> +	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.

> +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.

> +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.

> +	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.

> +			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.

> +			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.

> +		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?

> +		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;
> +	rta->rta_len = len;
> +	memcpy(RTA_DATA(rta), data, datalen);
> +	n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len);
> +
> +	return 0;
> +}
> +
> +static int prep_rtnl_newlink_req(struct iplink_req *req)
> +{
> +	char type[] = "caif";

Please don't do this. I think this is a waste.

> +	struct rtattr *linkinfo;
> +	struct rtattr *data;
> +
> +	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_NEWLINK;
> +	req->n.nlmsg_seq = ++rtnl_seqnr;
> +	req->i.ifi_family = AF_UNSPEC;
> +
> +	linkinfo = NLMSG_TAIL(&req->n);
> +	add_attribute(&req->n, sizeof(*req), IFLA_LINKINFO,
> +			NULL, 0);
> +	add_attribute(&req->n, sizeof(*req), IFLA_INFO_KIND,
> +			type, strlen(type));

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

> +	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.

> +	add_attribute(&req->n, sizeof(*req), IFLA_INFO_DATA,
> +			NULL, 0);
> +
> +	switch (req->type) {
> +

No empty line here.

> +	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;
> +}
> +
> +static void prep_rtnl_dellink_req(struct iplink_req *req, int ifindex)
> +{
> +	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.

> +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;
> +	}
> +
> +	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.

> +	parse_rtnl_message(buf, len);
> +
> +	return TRUE;
> +}
> +
> +int caif_rtnl_init(void)
> +{
> +	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.

> +	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);
> +
> +	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.

> +
> +	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);
> +	}
> +
> +	g_slist_free(pending_requests);
> +}
> +
> +
> +int caif_rtnl_create_interface(gpointer user_data, int type, int connid,
> +		gboolean loop, caif_rtnl_create_cb_t cb)
> +{
> +	int err;
> +	struct iplink_req *req;

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

> +
> +	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;
> +
> +	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.

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

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

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

Extra empty line before a label please.

> +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);
> +
> +	err = send_rtnl_req(&req);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}

Comments from above apply here.

> diff --git a/drivers/stemodem/caif_rtnl.h b/drivers/stemodem/caif_rtnl.h
> new file mode 100644
> index 0000000..8eb1e5f
> --- /dev/null
> +++ b/drivers/stemodem/caif_rtnl.h
> @@ -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
> + *  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
> + *
> + */
> +typedef void (*caif_rtnl_create_cb_t) (int result, gpointer user_data,
> +		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.

> +extern int caif_rtnl_create_interface(gpointer user_data, int type, int connid,
> +		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.

> +extern int caif_rtnl_delete_interface(int ifid);
> +
> +extern int caif_rtnl_init(void);
> +extern void caif_rtnl_exit(void);

Regards

Marcel

RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

[Sjur BRENDELAND]

[-- Attachment #1: Type: text/plain, Size: 16203 bytes --]

Hi Marcel.

> 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.

> > +#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.

>
> > +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...

>
> > +static guint  rtnl_watch;
>
> Get rid of the double spaces for rtnl_watch.
>
> > +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.
>

OK, Done.

>
> > +static void store_newlink_param(struct iplink_req *req, unsigned
> short type,
> > +                                   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.


Yes, I agree. I have squashed the two functions together and cleaned up the parameters.
This is much nicer, thank you.

>
> > +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.

OK, good I have squashed these to functions as well.

>
> > +static struct iplink_req *find_request(guint32 seq)
> > +{
> > +   GSList *list;
> > +
> > +   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.

OK, done.

>
> > +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.

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.

>
> > +static void parse_rtnl_message(void *buf, size_t len)
> > +{
> > +   struct ifinfomsg *msg;
> > +   struct iplink_req *req;
>
> Make it const void *buf.


OK, Done.

>
> 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.

>
> > +   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.

>
> > +                   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.

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

>
> > +                   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.

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.

>
> > +           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?

Yes, I have made the maxlen an unsigned int.

>
> > +   add_attribute(&req->n, sizeof(*req), IFLA_INFO_KIND,
> > +                   type, strlen(type));
>
> So add_attribute(..., "caif", 4) should be just fine.

OK, done.

>
> > +   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.

This is used for length calculation, I'll rename it to data_start.

>
> +static void prep_rtnl_dellink_req(struct iplink_req *req, int
> ifindex)
> > +{
> > +   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.

OK, done.

>
> > +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;
> > +   }
> > +
> > +   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.

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.

>
> > +   parse_rtnl_message(buf, len);
> > +
> > +   return TRUE;
> > +}
> > +
> > +int caif_rtnl_init(void)
> > +{
> > +   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.

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?

>
> > +   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);
> > +
> > +   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.

OK.

>
> > +
> > +   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);
> > +   }
> > +
> > +   g_slist_free(pending_requests);
> > +}
> > +
> > +
> > +int caif_rtnl_create_interface(gpointer user_data, int type, int
> connid,
> > +           gboolean loop, caif_rtnl_create_cb_t cb)
> > +{
> > +   int err;
> > +   struct iplink_req *req;
>
> I prefer structs before int. Just a minor style thing.

OK,

>
> > +
> > +   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;
> > +
> > +   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,

>
> 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...

>
> > +
> > +   err = send_rtnl_req(req);
> > +   if (err < 0)
> > +           goto error;
> > +
>
> Same here. Just putting the sendto() directly in here seems just fine.


OK, done.

>
> > +   pending_requests = g_slist_append(pending_requests, req);
> > +   return 0;
>
> Extra empty line before a label please.
>
> > +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);
> > +
> > +   err = send_rtnl_req(&req);
> > +   if (err < 0)
> > +           return err;
> > +
> > +   return 0;
> > +}
>
> Comments from above apply here.

OK, Done

>
> > diff --git a/drivers/stemodem/caif_rtnl.h
...
> > +typedef void (*caif_rtnl_create_cb_t) (int result, gpointer
> user_data,
> > +           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.


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

>
> > +extern int caif_rtnl_create_interface(gpointer user_data, int type,
> int connid,
> > +           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.
>

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....


Regards,
Sjur

RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 12242 bytes --]

Hi Sjur,

> > 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.

fall into a similar trap with GPRS support for IFX. 

> 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.

Sounds good to me.

Does anything cleans up the channel when oFono unexpectedly crashes?

> > > +#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.

I might just overread your response. Just wanted to make sure.

> > > +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...

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.

> > > +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.
> 
> 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.

> > 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.
> 
> >
> > > +   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.

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.

> > > +                   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.
> 
> OK - I'll break coding-style rule M12. There are more than 40 RTM messages
> I'm not handling, but no rule without exception :-)

Good point, but that rule does not apply here.

1) This is not our enum and comes from an external header file. We have
no control over and should not make such assumptions that M12 would
apply. It really only applies to our own enums. So using a default
statement is a good idea here.

2) This is part of wire protocol. And in that case you should always
write secure code to protect unexpected packets. So using a default is a
good idea as well.

Maybe someone needs to update our coding style document to include this.


> > > +                   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.
> 
> 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.

> > > +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;
> > > +   }
> > > +
> > > +   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.
> 
> 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.

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.

> > > +   parse_rtnl_message(buf, len);
> > > +
> > > +   return TRUE;
> > > +}
> > > +
> > > +int caif_rtnl_init(void)
> > > +{
> > > +   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.
> 
> 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?

Yes. You should not mess with IP addresses.

> > > +
> > > +   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;
> > > +
> > > +   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,

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.

> > 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...

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.

> > > diff --git a/drivers/stemodem/caif_rtnl.h
> ...
> > > +typedef void (*caif_rtnl_create_cb_t) (int result, gpointer
> > user_data,
> > > +           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.
> 
> 
> 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).

> Thank you for a very thorough review here Marcel.

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

> 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....

Sounds good.

Regards

Marcel

RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

[Sjur BRENDELAND]

[-- Attachment #1: Type: text/plain, Size: 5986 bytes --]

Hi Marcel.

> Does anything cleans up the channel when oFono unexpectedly crashes?

No not yet. I suppose the interface will continue up until someone
destroys it. Should we simply destroy all CAIF interfaces
if oFono crashes? I think this would be the simplest approach.

BTW, how does connman behave in this case? Will it do any attempt
to clean up IP interfaces when it notices that oFono APIs has gone missing?
And what about modem state, is it taken back to flight-mode (AT+CFUN=0) in
order to reset everything?

If you don't mind, I would prefer to do restart handling as the next step.
It would be nice to finish this patch review first, what do you think?

> 
> 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.

Yes, done.

> > 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.

OK, tell me what you think about the next patch then.

> > 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.
> 
> 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.

The seqnr is not present in the message header of this response message,
so this will not be possible. I would prefer to stick with the current
implementation. Or do you see another way forward?
 
> > > > +                   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.
> >
> > 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.

OK, I think I have done so. Have a look when I send the next patch.

> > 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.
> 
> 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.

OK, I've changed it to recv.

> > > 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,
> 
> 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.

OK, I've squashed the two functions. 

> 
> > > 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...
> 
> 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.

OK, done. This struct is much smaller now.

> 
> > > > diff --git a/drivers/stemodem/caif_rtnl.h
> > ...
> > > > +typedef void (*caif_rtnl_create_cb_t) (int result, gpointer
> > > user_data,
> > > > +           char *ifname, int ifindex);
...
> > 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).

OK, done.

> > Thank you for a very thorough review here Marcel.
> 
> No problem. Just RTNL takes always a bit to get used to again ;)

Yes, ditto.  Going from ioctl with just a couple of function calls into rtnl
has been surprisingly complex.

Regards,
Sjur