[PATCH 1/4] stemodem: Add rtnl header file.

[PATCH 1/4] stemodem: Add rtnl header file.

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

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

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

Add new asynchronous rtnl interface for creating and deleting
CAIF Network interfaces.

Creation takes struct rtnl_req_param as input containing
ip type (v4/v6), channel id etc. The result is returned in
struct rtnl_rsp_param containing the interface name and
interface index.

---
 drivers/stemodem/rtnl.h |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)
 create mode 100644 drivers/stemodem/rtnl.h

diff --git a/drivers/stemodem/rtnl.h b/drivers/stemodem/rtnl.h
new file mode 100644
index 0000000..566452b
--- /dev/null
+++ b/drivers/stemodem/rtnl.h
@@ -0,0 +1,41 @@
+/*
+ *
+ *  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
+ *
+ */
+struct rtnl_rsp_param {
+	int result;
+	gpointer user_data;
+	char ifname[IF_NAMESIZE];
+	int  ifindex;
+};
+
+struct rtnl_req_param {
+	void (*callback)(struct rtnl_rsp_param *param);
+	gpointer user_data;
+	int type;
+	int conn_id;
+	gboolean loop_enabled;
+};
+
+extern int rtnl_create_caif_interface(struct rtnl_req_param *req);
+extern int rtnl_delete_caif_interface(int ifid);
+
+extern int rtnl_init(void);
+extern void rtnl_exit(void);
+
-- 
1.6.3.3

[PATCH 2/4] stemodem: Add RTNL functionality for CAIF Netw Interface.

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

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

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

Add file rtnl.c for creating and deleting CAIF network
interfaces using the RTNL protocol. The interface is
asynchronous.

Only RTNL NEWLINK and DELLINK commands are implemented.
CAIF requires NEWLINK to contain channel-id and ip-type
(ipv4/ipv6) as arguments.

---
 drivers/stemodem/rtnl.c |  365 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 365 insertions(+), 0 deletions(-)
 create mode 100644 drivers/stemodem/rtnl.c

diff --git a/drivers/stemodem/rtnl.c b/drivers/stemodem/rtnl.c
new file mode 100644
index 0000000..38f7f43
--- /dev/null
+++ b/drivers/stemodem/rtnl.c
@@ -0,0 +1,365 @@
+/*
+ *
+ *  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 <linux/rtnetlink.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+
+#include "if_caif.h"
+#include "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;
+	struct rtnl_req_param req;
+	struct rtnl_rsp_param rsp;
+};
+
+static GSList *pending_requests;
+static GIOChannel *channel;
+static guint32 rtnl_seqnr;
+static guint  rtnl_watch;
+
+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->rsp.ifname, ifname,
+			sizeof(req->rsp.ifname));
+	req->rsp.ifname[sizeof(req->rsp.ifname)-1] = '\0';
+	req->rsp.ifindex = index;
+}
+
+static int send_rtnl_req(struct iplink_req *req)
+{
+	struct sockaddr_nl addr;
+	int sk, ret;
+
+	sk = g_io_channel_unix_get_fd(channel);
+
+	memset(&addr, 0, sizeof(addr));
+	addr.nl_family = AF_NETLINK;
+
+	ret = sendto(sk, req, req->n.nlmsg_len, 0,
+			(struct sockaddr *) &addr, sizeof(addr));
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+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)
+{
+
+	if (req->req.callback && req->n.nlmsg_type == RTM_NEWLINK)
+		req->req.callback(&req->rsp);
+
+	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);
+			DBG("NEWLINK req:%p\n", req);
+			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);
+			return;
+
+		} else if (hdr->nlmsg_type == NLMSG_ERROR) {
+			req = find_request(hdr->nlmsg_seq);
+			DBG("NLMSG ERROR req:%p\n", req);
+			if (req == NULL)
+				break;
+			req->rsp.result = -1;
+			rtnl_client_response(req);
+			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,
+					struct rtnl_req_param *param)
+{
+	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);
+
+	if (param->type == IFLA_CAIF_IPV4_CONNID)
+		add_attribute(&req->n, sizeof(*req),
+				IFLA_CAIF_IPV4_CONNID, &param->conn_id,
+				sizeof(param->conn_id));
+	else if (param->type == IFLA_CAIF_IPV6_CONNID)
+		add_attribute(&req->n, sizeof(*req),
+				IFLA_CAIF_IPV6_CONNID, &param->conn_id,
+				sizeof(param->conn_id));
+	else {
+		DBG("unsupported linktype\n");
+		return -EINVAL;
+	}
+
+	if (param->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))
+		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;
+		return FALSE;
+	}
+
+	parse_rtnl_message(buf, len);
+
+	return TRUE;
+}
+
+int rtnl_init(void)
+{
+	struct sockaddr_nl addr;
+	int sk, ret;
+
+	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;
+
+	ret = bind(sk, (struct sockaddr *) &addr, sizeof(addr));
+	if (ret < 0) {
+		close(sk);
+		return ret;
+	}
+
+	channel = g_io_channel_unix_new(sk);
+	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);
+
+	return 0;
+}
+
+void rtnl_exit(void)
+{
+	GSList *list;
+
+	if (rtnl_watch > 0)
+		g_source_remove(rtnl_watch);
+
+	for (list = pending_requests; list; list = list->next) {
+		struct iplink_req *req = list->data;
+		g_free(req);
+		list->data = NULL;
+	}
+
+	g_slist_free(pending_requests);
+	pending_requests = NULL;
+
+	g_io_channel_shutdown(channel, TRUE, NULL);
+	g_io_channel_unref(channel);
+
+	channel = NULL;
+}
+
+
+int rtnl_create_caif_interface(struct rtnl_req_param *req_param)
+{
+	int ret;
+	struct rtnl_rsp_param resp_param;
+	struct iplink_req *req;
+
+	memset(&resp_param, 0, sizeof(resp_param));
+
+	req = g_try_new0(struct iplink_req, 1);
+	if (req == NULL) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	resp_param.user_data = req_param->user_data;
+
+	req->req = *req_param;
+	req->rsp = resp_param;
+
+	ret = prep_rtnl_newlink_req(req, req_param);
+	if (ret < 0)
+		goto error;
+
+	pending_requests = g_slist_append(pending_requests, req);
+
+	ret = send_rtnl_req(req);
+	if (ret == 0)
+		return 0;
+
+	pending_requests = g_slist_remove(pending_requests, req);
+error:
+	g_free(req);
+	return ret;
+}
+
+int rtnl_delete_caif_interface(int ifid)
+{
+	struct iplink_req req;
+
+	memset(&req, 0, sizeof(req));
+	prep_rtnl_dellink_req(&req, ifid);
+	return send_rtnl_req(&req);
+}
-- 
1.6.3.3

Re: [PATCH 2/4] stemodem: Add RTNL functionality for CAIF Netw Interface.

[Marcel Holtmann]

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

Hi Sjur,

> Add file rtnl.c for creating and deleting CAIF network
> interfaces using the RTNL protocol. The interface is
> asynchronous.
> 
> Only RTNL NEWLINK and DELLINK commands are implemented.
> CAIF requires NEWLINK to contain channel-id and ip-type
> (ipv4/ipv6) as arguments.
> 
> ---
>  drivers/stemodem/rtnl.c |  365 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 365 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/stemodem/rtnl.c

I have no problems with the file names. It does make sense to keep them
simple. However if this is CAIF related, then using caif_rtnl.[ch] might
be a good idea.

And you can combine the patches for the header file, for the actual code
and the Makefile.am into one. That is fine.

<snip>

And I have not looked through the whole patch yet.

> +int rtnl_init(void)
> +{
> +	struct sockaddr_nl addr;
> +	int sk, ret;
> +
> +	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;
> +
> +	ret = bind(sk, (struct sockaddr *) &addr, sizeof(addr));
> +	if (ret < 0) {
> +		close(sk);
> +		return ret;
> +	}
> +
> +	channel = g_io_channel_unix_new(sk);
> +	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);
> +
> +	return 0;
> +}
> +
> +void rtnl_exit(void)
> +{
> +	GSList *list;
> +
> +	if (rtnl_watch > 0)
> +		g_source_remove(rtnl_watch);
> +
> +	for (list = pending_requests; list; list = list->next) {
> +		struct iplink_req *req = list->data;
> +		g_free(req);
> +		list->data = NULL;
> +	}
> +
> +	g_slist_free(pending_requests);
> +	pending_requests = NULL;
> +
> +	g_io_channel_shutdown(channel, TRUE, NULL);
> +	g_io_channel_unref(channel);
> +
> +	channel = NULL;
> +}

Using global function names like rtnl_init and rtnl_exit is not gonna
cut it. This code is CAIF specific and you need at least prefix the
public functions properly.

I am not sure what is best here. Use caif_rtnl_ or ste_rtnl_ or maybe
just skip the keyword rtnl and just prefix it with caif_ only.

Regards

Marcel

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

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

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

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

---
Changes from last patch-set:
o squashed previous patches for rtnl.c, rtnl.h and Makefile.am into one patch
o renamed rtnl.[ch] to caif_rtnl.[ch]
o renamed rtnl_init and rtnl_exit to caif_rtnl_*
o renamed rtnl_create_caif_interface to caif_rtnl_create_interface.

 Makefile.am                  |    2 +
 drivers/stemodem/caif_rtnl.c |  365 ++++++++++++++++++++++++++++++++++++++++++
 drivers/stemodem/caif_rtnl.h |   40 +++++
 3 files changed, 407 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 2562160..fdd363d 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..2712df7
--- /dev/null
+++ b/drivers/stemodem/caif_rtnl.c
@@ -0,0 +1,365 @@
+/*
+ *
+ *  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 <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;
+	struct rtnl_req_param req;
+	struct rtnl_rsp_param rsp;
+};
+
+static GSList *pending_requests;
+static GIOChannel *channel;
+static guint32 rtnl_seqnr;
+static guint  rtnl_watch;
+
+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->rsp.ifname, ifname,
+			sizeof(req->rsp.ifname));
+	req->rsp.ifname[sizeof(req->rsp.ifname)-1] = '\0';
+	req->rsp.ifindex = index;
+}
+
+static int send_rtnl_req(struct iplink_req *req)
+{
+	struct sockaddr_nl addr;
+	int sk, ret;
+
+	sk = g_io_channel_unix_get_fd(channel);
+
+	memset(&addr, 0, sizeof(addr));
+	addr.nl_family = AF_NETLINK;
+
+	ret = sendto(sk, req, req->n.nlmsg_len, 0,
+			(struct sockaddr *) &addr, sizeof(addr));
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+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)
+{
+
+	if (req->req.callback && req->n.nlmsg_type == RTM_NEWLINK)
+		req->req.callback(&req->rsp);
+
+	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);
+			DBG("NEWLINK req:%p\n", req);
+			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);
+			return;
+
+		} else if (hdr->nlmsg_type == NLMSG_ERROR) {
+			req = find_request(hdr->nlmsg_seq);
+			DBG("NLMSG ERROR req:%p\n", req);
+			if (req == NULL)
+				break;
+			req->rsp.result = -1;
+			rtnl_client_response(req);
+			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,
+					struct rtnl_req_param *param)
+{
+	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);
+
+	if (param->type == IFLA_CAIF_IPV4_CONNID)
+		add_attribute(&req->n, sizeof(*req),
+				IFLA_CAIF_IPV4_CONNID, &param->conn_id,
+				sizeof(param->conn_id));
+	else if (param->type == IFLA_CAIF_IPV6_CONNID)
+		add_attribute(&req->n, sizeof(*req),
+				IFLA_CAIF_IPV6_CONNID, &param->conn_id,
+				sizeof(param->conn_id));
+	else {
+		DBG("unsupported linktype\n");
+		return -EINVAL;
+	}
+
+	if (param->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))
+		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;
+		return FALSE;
+	}
+
+	parse_rtnl_message(buf, len);
+
+	return TRUE;
+}
+
+int caif_rtnl_init(void)
+{
+	struct sockaddr_nl addr;
+	int sk, ret;
+
+	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;
+
+	ret = bind(sk, (struct sockaddr *) &addr, sizeof(addr));
+	if (ret < 0) {
+		close(sk);
+		return ret;
+	}
+
+	channel = g_io_channel_unix_new(sk);
+	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);
+
+	return 0;
+}
+
+void caif_rtnl_exit(void)
+{
+	GSList *list;
+
+	if (rtnl_watch > 0)
+		g_source_remove(rtnl_watch);
+
+	for (list = pending_requests; list; list = list->next) {
+		struct iplink_req *req = list->data;
+		g_free(req);
+		list->data = NULL;
+	}
+
+	g_slist_free(pending_requests);
+	pending_requests = NULL;
+
+	g_io_channel_shutdown(channel, TRUE, NULL);
+	g_io_channel_unref(channel);
+
+	channel = NULL;
+}
+
+
+int caif_rtnl_create_interface(struct rtnl_req_param *req_param)
+{
+	int ret;
+	struct rtnl_rsp_param resp_param;
+	struct iplink_req *req;
+
+	memset(&resp_param, 0, sizeof(resp_param));
+
+	req = g_try_new0(struct iplink_req, 1);
+	if (req == NULL) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	resp_param.user_data = req_param->user_data;
+
+	req->req = *req_param;
+	req->rsp = resp_param;
+
+	ret = prep_rtnl_newlink_req(req, req_param);
+	if (ret < 0)
+		goto error;
+
+	pending_requests = g_slist_append(pending_requests, req);
+
+	ret = send_rtnl_req(req);
+	if (ret == 0)
+		return 0;
+
+	pending_requests = g_slist_remove(pending_requests, req);
+error:
+	g_free(req);
+	return ret;
+}
+
+int caif_rtnl_delete_interface(int ifid)
+{
+	struct iplink_req req;
+
+	memset(&req, 0, sizeof(req));
+	prep_rtnl_dellink_req(&req, ifid);
+	return send_rtnl_req(&req);
+}
diff --git a/drivers/stemodem/caif_rtnl.h b/drivers/stemodem/caif_rtnl.h
new file mode 100644
index 0000000..3d171f9
--- /dev/null
+++ b/drivers/stemodem/caif_rtnl.h
@@ -0,0 +1,40 @@
+/*
+ *
+ *  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
+ *
+ */
+struct rtnl_rsp_param {
+	int result;
+	gpointer user_data;
+	char ifname[IF_NAMESIZE];
+	int  ifindex;
+};
+
+struct rtnl_req_param {
+	void (*callback)(struct rtnl_rsp_param *param);
+	gpointer user_data;
+	int type;
+	int conn_id;
+	gboolean loop_enabled;
+};
+
+extern int caif_rtnl_create_interface(struct rtnl_req_param *req);
+extern int caif_rtnl_delete_interface(int ifid);
+
+extern int caif_rtnl_init(void);
+extern void caif_rtnl_exit(void);
-- 
1.7.0.4

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

[Marcel Holtmann]

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

Hi Sjur,

>  Makefile.am                  |    2 +
>  drivers/stemodem/caif_rtnl.c |  365 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/stemodem/caif_rtnl.h |   40 +++++
>  3 files changed, 407 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/stemodem/caif_rtnl.c
>  create mode 100644 drivers/stemodem/caif_rtnl.h

just to be clear. You want to name it like this? I am just asking here
since I made a few proposals.

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

We don't have this one as part of some kernel includes?

> +struct iplink_req {
> +	struct nlmsghdr n;
> +	struct ifinfomsg i;
> +	char pad[1024];
> +	int request_id;
> +	struct rtnl_req_param req;
> +	struct rtnl_rsp_param rsp;
> +};
> +
> +static GSList *pending_requests;
> +static GIOChannel *channel;
> +static guint32 rtnl_seqnr;
> +static guint  rtnl_watch;

To be fair, you don't need both, the GIOChannel and the watch. You can
just add the watch and then unref the GIOChannel and set it to close on
unref. That way when the watch gets removed or you remove it, the
GIOChannel and the underlaying socket will be automatically closed as
well.

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

After I stared long enough at it, I realized that the code is fine, but
that was not obvious to me. In this case please use for () { } with the
curly braces around it. It improves readability.

And I know that other times we don't do { } for single statements, but
here is does helps my poor human brain ;)


> +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->rsp.ifname, ifname,
> +			sizeof(req->rsp.ifname));
> +	req->rsp.ifname[sizeof(req->rsp.ifname)-1] = '\0';
> +	req->rsp.ifindex = index;
> +}
> +
> +static int send_rtnl_req(struct iplink_req *req)
> +{
> +	struct sockaddr_nl addr;
> +	int sk, ret;
> +
> +	sk = g_io_channel_unix_get_fd(channel);
> +
> +	memset(&addr, 0, sizeof(addr));
> +	addr.nl_family = AF_NETLINK;
> +
> +	ret = sendto(sk, req, req->n.nlmsg_len, 0,
> +			(struct sockaddr *) &addr, sizeof(addr));
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}

Can we just not do "return sendto(..." and then have the caller to a
proper "if send_rtnl_req() < 0)" check. Looks much simpler to me.

> +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)
> +{
> +
> +	if (req->req.callback && req->n.nlmsg_type == RTM_NEWLINK)
> +		req->req.callback(&req->rsp);
> +
> +	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);
> +			DBG("NEWLINK req:%p\n", req);
> +			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);
> +			return;
> +
> +		} else if (hdr->nlmsg_type == NLMSG_ERROR) {
> +			req = find_request(hdr->nlmsg_seq);
> +			DBG("NLMSG ERROR req:%p\n", req);
> +			if (req == NULL)
> +				break;
> +			req->rsp.result = -1;
> +			rtnl_client_response(req);
> +			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,
> +					struct rtnl_req_param *param)
> +{
> +	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);
> +
> +

You have double empty lines here (twice). Please remove them.

> +	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);
> +
> +	if (param->type == IFLA_CAIF_IPV4_CONNID)
> +		add_attribute(&req->n, sizeof(*req),
> +				IFLA_CAIF_IPV4_CONNID, &param->conn_id,
> +				sizeof(param->conn_id));
> +	else if (param->type == IFLA_CAIF_IPV6_CONNID)
> +		add_attribute(&req->n, sizeof(*req),
> +				IFLA_CAIF_IPV6_CONNID, &param->conn_id,
> +				sizeof(param->conn_id));
> +	else {
> +		DBG("unsupported linktype\n");
> +		return -EINVAL;
> +	}

Would be using a switch here a bit cleaner? Please use a switch.

> +	if (param->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))
> +		return FALSE;

In this return case you need to set the global watch variable back to 0
since that watch will be gone now.

> +
> +	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;
> +		return FALSE;

Same case here.

> +	}
> +
> +	parse_rtnl_message(buf, len);
> +
> +	return TRUE;
> +}
> +
> +int caif_rtnl_init(void)
> +{
> +	struct sockaddr_nl addr;
> +	int sk, ret;
> +
> +	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;
> +
> +	ret = bind(sk, (struct sockaddr *) &addr, sizeof(addr));
> +	if (ret < 0) {
> +		close(sk);
> +		return ret;
> +	}
> +
> +	channel = g_io_channel_unix_new(sk);
> +	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);

As described above. Just unref the GIOChannel and forget about it. And
then you can make the GIOChannel a local variable.

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

Setting list->data = NULL is not really needed.

> +	}
> +
> +	g_slist_free(pending_requests);
> +	pending_requests = NULL;

No need to set pending_requests to NULL here.

> +
> +	g_io_channel_shutdown(channel, TRUE, NULL);
> +	g_io_channel_unref(channel);
> +
> +	channel = NULL;

As described above, this is no longer needed.
> +}
> +
> +
> +int caif_rtnl_create_interface(struct rtnl_req_param *req_param)
> +{
> +	int ret;

Please use err as variable instead of ret.

> +	struct rtnl_rsp_param resp_param;
> +	struct iplink_req *req;
> +
> +	memset(&resp_param, 0, sizeof(resp_param));
> +
> +	req = g_try_new0(struct iplink_req, 1);
> +	if (req == NULL) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}

This is rather pointless. Just to "return -ENOMEM;"

> +	resp_param.user_data = req_param->user_data;
> +
> +	req->req = *req_param;
> +	req->rsp = resp_param;
> +
> +	ret = prep_rtnl_newlink_req(req, req_param);
> +	if (ret < 0)
> +		goto error;
> +
> +	pending_requests = g_slist_append(pending_requests, req);
> +
> +	ret = send_rtnl_req(req);
> +	if (ret == 0)
> +		return 0;

As mentioned above, this should be if (err < 0).

> +
> +	pending_requests = g_slist_remove(pending_requests, req);

You don't really have to play this append/remove game. You can just
append it after send_rtnl_req succeed. The read goes via the mainloop
anyway and we are a single threaded program. So the order is guaranteed.

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

And here you should add an extra empty line.

> +	return send_rtnl_req(&req);
> +}
> diff --git a/drivers/stemodem/caif_rtnl.h b/drivers/stemodem/caif_rtnl.h
> new file mode 100644
> index 0000000..3d171f9
> --- /dev/null
> +++ b/drivers/stemodem/caif_rtnl.h
> @@ -0,0 +1,40 @@
> +/*
> + *
> + *  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
> + *
> + */

And an extra empty line here to separate copyright header and license
text from actual code.

> +struct rtnl_rsp_param {
> +	int result;
> +	gpointer user_data;
> +	char ifname[IF_NAMESIZE];
> +	int  ifindex;
> +};
> +
> +struct rtnl_req_param {
> +	void (*callback)(struct rtnl_rsp_param *param);
> +	gpointer user_data;
> +	int type;
> +	int conn_id;
> +	gboolean loop_enabled;
> +};
> +
> +extern int caif_rtnl_create_interface(struct rtnl_req_param *req);
> +extern int caif_rtnl_delete_interface(int ifid);

I am not really sold on this param stuct thingy btw. Why do you need
them? Just proper input params and output params for the callback
handler would do them same. Won't they?

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

I do need another look at the RTNL magic and casting. That always drives
my crazy when I look at that. In the meantime, please address these
details first.

Regards

Marcel

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

[Sjur BRENDELAND]

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

> From: Marcel Holtmann [mailto:marcel(a)holtmann.org]
> 
> just to be clear. You want to name it like this? I am just asking here
> since I made a few proposals.

No worries :-)
The file is called caif_rtnl and exported functions are prefixed caif_rtnl,
I think that makes a lot of sense.

> 
> > +#define NLMSG_TAIL(nmsg) \
> > +	((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-
> >nlmsg_len)))
> 
> We don't have this one as part of some kernel includes?

I'll check what I can find ;-)

> 
> > +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;
> > +}
> 
> After I stared long enough at it, I realized that the code is fine, but
> that was not obvious to me. In this case please use for () { } with the
> curly braces around it. It improves readability.
> 
> And I know that other times we don't do { } for single statements, but
> here is does helps my poor human brain ;)

Sure I'll add the braces ;-)

> You don't really have to play this append/remove game. You can just
> append it after send_rtnl_req succeed. The read goes via the mainloop
> anyway and we are a single threaded program. So the order is
> guaranteed.

I'm not sure about this. I need to keep track of what
connection-id I assign to the CAIF Interface in the request and what 
interface name and interface index I get for that connection id
in the RTNL response.
This mapping between network interface and connection id is used 
when PDP Contexts are activated. The append/remove game provides me
with the request-response mapping. 


> And here you should add an extra empty line.

Sorry, I don't know why I keep doing that...

> > +struct rtnl_rsp_param {
> > +	int result;
> > +	gpointer user_data;
> > +	char ifname[IF_NAMESIZE];
> > +	int  ifindex;
> > +};
> > +
> > +struct rtnl_req_param {
> > +	void (*callback)(struct rtnl_rsp_param *param);
> > +	gpointer user_data;
> > +	int type;
> > +	int conn_id;
> > +	gboolean loop_enabled;
> > +};
> > +
> > +extern int caif_rtnl_create_interface(struct rtnl_req_param *req);
> > +extern int caif_rtnl_delete_interface(int ifid);
> 
> I am not really sold on this param stuct thingy btw. Why do you need
> them? Just proper input params and output params for the callback
> handler would do them same. Won't they?

Yes, I could do that. But in that case I would prefer to typedef the 
response function to simplify create function's signature. Otherwise
I think the create function's signature gets too complex.

...
> I do need another look at the RTNL magic and casting. That always
> drives
> my crazy when I look at that. In the meantime, please address these
> details first.

Sure, and thanks for reviewing.

Regards,
Sjur

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

[Marcel Holtmann]

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

Hi Sjur,

> > You don't really have to play this append/remove game. You can just
> > append it after send_rtnl_req succeed. The read goes via the mainloop
> > anyway and we are a single threaded program. So the order is
> > guaranteed.
> 
> I'm not sure about this. I need to keep track of what
> connection-id I assign to the CAIF Interface in the request and what 
> interface name and interface index I get for that connection id
> in the RTNL response.
> This mapping between network interface and connection id is used 
> when PDP Contexts are activated. The append/remove game provides me
> with the request-response mapping. 

I know what you are trying to protect here, but what I am saying is that
just appending the struct to the list after you send it (and evaluated
its error code) is just fine.

You don't need to append it first, send the message, check error and in
case of an error remove it again.

The nature of single-thread application using a mainloop and where input
is driven by that mainloop, ensures that you will only receive the reply
to your message after you added it to the list. Even if that is done
after you send it.

So while it is fine to append the structure to the list for further
tracking, you should just do it after you send the message and it
succeeded and be done with it.

And while we are at it. Essentially you also need to use asynchronous
and non-blocking watch driven sending of your message in the first
place. I did forget to check if you open the RTNL socket non-blocking,
but you should be doing that.

> > > +struct rtnl_rsp_param {
> > > +	int result;
> > > +	gpointer user_data;
> > > +	char ifname[IF_NAMESIZE];
> > > +	int  ifindex;
> > > +};
> > > +
> > > +struct rtnl_req_param {
> > > +	void (*callback)(struct rtnl_rsp_param *param);
> > > +	gpointer user_data;
> > > +	int type;
> > > +	int conn_id;
> > > +	gboolean loop_enabled;
> > > +};
> > > +
> > > +extern int caif_rtnl_create_interface(struct rtnl_req_param *req);
> > > +extern int caif_rtnl_delete_interface(int ifid);
> > 
> > I am not really sold on this param stuct thingy btw. Why do you need
> > them? Just proper input params and output params for the callback
> > handler would do them same. Won't they?
> 
> Yes, I could do that. But in that case I would prefer to typedef the 
> response function to simplify create function's signature. Otherwise
> I think the create function's signature gets too complex.

typedef for function callbacks are just fine. I have no problem with
that at all.

I am not set in stone here, but for something this simple, it seems to
me that the two structs is a bit of overhead and complexity that we
could avoid.

Regards

Marcel

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

[Sjur BRENDELAND]

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

Hi Marcel.

> > +static GSList *pending_requests;
> > +static GIOChannel *channel;
> > +static guint32 rtnl_seqnr;
> > +static guint  rtnl_watch;
> 
> To be fair, you don't need both, the GIOChannel and the watch. You can
> just add the watch and then unref the GIOChannel and set it to close on
> unref. That way when the watch gets removed or you remove it, the
> GIOChannel and the underlaying socket will be automatically closed as
> well.
....
> As described above. Just unref the GIOChannel and forget about it. And
> then you can make the GIOChannel a local variable.

I think I have to keep the channel, because I do need the
file descriptor derived from the channel when doing sendto.

> > > You don't really have to play this append/remove game. You can just
> > > append it after send_rtnl_req succeed. The read goes via the
> mainloop
> > > anyway and we are a single threaded program. So the order is
> > > guaranteed.
...
> You don't need to append it first, send the message, check error and in
> case of an error remove it again.

OK, thanks - now I get what your after. I completely misunderstood your 
previous comment, my bad.

> And while we are at it. Essentially you also need to use asynchronous
> and non-blocking watch driven sending of your message in the first
> place. I did forget to check if you open the RTNL socket non-blocking,
> but you should be doing that.

So I'll add something like this then:
	fl = fcntl(sk, F_GETFL);
	fcntl(sk, F_SETFL, fl | O_NONBLOCK);


> > +	}
> > +
> > +	g_slist_free(pending_requests);
> > +	pending_requests = NULL;
> 
> No need to set pending_requests to NULL here.

But I will have to move this to the caif_rtnl_init function then,
I think pending_requests needs to be NULL before using it.

> > > I am not really sold on this param stuct thingy btw. Why do you
> need
> > > them? Just proper input params and output params for the callback
> > > handler would do them same. Won't they?
> >
> > Yes, I could do that. But in that case I would prefer to typedef the
> > response function to simplify create function's signature. Otherwise
> > I think the create function's signature gets too complex.
> 
> typedef for function callbacks are just fine. I have no problem with
> that at all.

I got rid of the structs and typedef'ed response function - I agree this better.

<snip>
typedef void (*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, rtnl_create_cb_t cb);


Hoping to get the next patch out later today.

Regards,
Sjur

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

[Marcel Holtmann]

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

Hi Sjur,

> > > +static GSList *pending_requests;
> > > +static GIOChannel *channel;
> > > +static guint32 rtnl_seqnr;
> > > +static guint  rtnl_watch;
> > 
> > To be fair, you don't need both, the GIOChannel and the watch. You can
> > just add the watch and then unref the GIOChannel and set it to close on
> > unref. That way when the watch gets removed or you remove it, the
> > GIOChannel and the underlaying socket will be automatically closed as
> > well.
> ....
> > As described above. Just unref the GIOChannel and forget about it. And
> > then you can make the GIOChannel a local variable.
> 
> I think I have to keep the channel, because I do need the
> file descriptor derived from the channel when doing sendto.

fair enough. Anything wrong with just using g_io_channel_write_chars and
setting the channel to not-buffered and no-encoding?

> > > > You don't really have to play this append/remove game. You can just
> > > > append it after send_rtnl_req succeed. The read goes via the
> > mainloop
> > > > anyway and we are a single threaded program. So the order is
> > > > guaranteed.
> ...
> > You don't need to append it first, send the message, check error and in
> > case of an error remove it again.
> 
> OK, thanks - now I get what your after. I completely misunderstood your 
> previous comment, my bad.

That is what I figured. I think that I wasn't clear enough here.

> > And while we are at it. Essentially you also need to use asynchronous
> > and non-blocking watch driven sending of your message in the first
> > place. I did forget to check if you open the RTNL socket non-blocking,
> > but you should be doing that.
> 
> So I'll add something like this then:
> 	fl = fcntl(sk, F_GETFL);
> 	fcntl(sk, F_SETFL, fl | O_NONBLOCK);

You can also just use the one from GIOChannel to do this since you wrap
it into a GIOChannel right away anyway.

> > > +	}
> > > +
> > > +	g_slist_free(pending_requests);
> > > +	pending_requests = NULL;
> > 
> > No need to set pending_requests to NULL here.
> 
> But I will have to move this to the caif_rtnl_init function then,
> I think pending_requests needs to be NULL before using it.

That is given by the fact that it is a global static variable. And when
the exit gets called, we are ending the ofonod.

Not a big thing. Just a minor nitpick. And if you look you will find
cases where even I kept doing it ;)

> > > > I am not really sold on this param stuct thingy btw. Why do you
> > need
> > > > them? Just proper input params and output params for the callback
> > > > handler would do them same. Won't they?
> > >
> > > Yes, I could do that. But in that case I would prefer to typedef the
> > > response function to simplify create function's signature. Otherwise
> > > I think the create function's signature gets too complex.
> > 
> > typedef for function callbacks are just fine. I have no problem with
> > that at all.
> 
> I got rid of the structs and typedef'ed response function - I agree this better.
> 
> <snip>
> typedef void (*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, rtnl_create_cb_t cb);
> 
> 
> Hoping to get the next patch out later today.

Please don't use rtnl_ prefix. That is not your namespace. So it should
be caif_rtnl_create_cb.

Regards

Marcel

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

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

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

Hi Marcel.

> I do need another look at the RTNL magic and casting. That always drives
> my crazy when I look at that. In the meantime, please address these
> details first.

I have some RTNL unit tests ready if your're interested.
Also I hope I have closed most open issues from your review
in the v4 version of this patch, please inform me if you think
otherwise.

Regards,
Sjur

[PATCHv2 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: 12134 bytes --]

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

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

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index d3a50a9..e2b7e54 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,64 @@ 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(struct rtnl_rsp_param *param)
+{
+	struct conn_info *conn = param->user_data;
+
+	strncpy(conn->interface, param->ifname, sizeof(conn->interface));
+	conn->ifindex = param->ifindex;
+
+	if (param->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;
+	struct rtnl_req_param req_param = {
+		.type = IFLA_CAIF_IPV4_CONNID,
+		.conn_id = conn->channel_id,
+		.user_data = conn,
+		.callback = rtnl_callback,
+		.loop_enabled = FALSE
+	};
+
+	if (caif_rtnl_create_interface(&req_param) < 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 +239,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 +260,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 +279,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 +312,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 +329,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 +343,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 +355,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 +379,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 +407,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 +416,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 +447,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 +456,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 +485,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 +578,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 +591,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 +612,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

[PATCH 3/4] stemodem: Update gprs-context to use rtnl

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

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

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

RTNL is now used to create and remove CAIF network interfaces.
The created CAIF interface is assigned a channel-id which is
used as parameter in the AT*EPPSD command used to activate the
PDP-Context.

The CAIF Network interfaces are created when gprs-context is probed
initially.

Some refactoring and bug-fixes are also included.

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

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index d3a50a9..7e956d3 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 "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,64 @@ 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(struct rtnl_rsp_param *param)
+{
+	struct conn_info *conn = param->user_data;
+
+	strncpy(conn->interface, param->ifname, sizeof(conn->interface));
+	conn->ifindex = param->ifindex;
+
+	if (param->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;
+	struct rtnl_req_param req_param = {
+		.type = IFLA_CAIF_IPV4_CONNID,
+		.conn_id = conn->channel_id,
+		.user_data = conn,
+		.callback = rtnl_callback,
+		.loop_enabled = FALSE
+	};
+
+	if (rtnl_create_caif_interface(&req_param) < 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 (rtnl_delete_caif_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 +239,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 +260,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 +279,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 +312,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 +329,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 +343,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 +355,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 +379,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 +407,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 +416,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 +447,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 +456,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 +485,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 +578,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 +591,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 +612,12 @@ static struct ofono_gprs_context_driver driver = {
 
 void ste_gprs_context_init()
 {
+	rtnl_init();
 	ofono_gprs_context_driver_register(&driver);
 }
 
 void ste_gprs_context_exit()
 {
+	rtnl_exit();
 	ofono_gprs_context_driver_unregister(&driver);
 }
-- 
1.6.3.3

[PATCH 4/4] stemodem: Add rtnl to Makefile.

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

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

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

---
 Makefile.am |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 2562160..1b09a3c 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/rtnl.c \
+			drivers/stemodem/rtnl.h \
 			drivers/stemodem/gprs-context.c \
 			drivers/stemodem/caif_socket.h \
 			drivers/stemodem/if_caif.h
-- 
1.6.3.3

Re: [PATCH 4/4] stemodem: Add rtnl to Makefile.

[Marcel Holtmann]

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

Hi Sjur,

> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> ---
>  Makefile.am |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

this one need to come before you actually use that code of course ;)

And it is so simple, just feed it into the initial patch adding the RTNL
support.

Regards

Marcel