Skip to contentSkip to conversation

stemodem: Add rtnl header file.

PATCH In review Sjur Brændeland 3 participants 14 messages Oct 28, 2010 – Nov 9, 2010 Kbuild

lore
Share & tools
lore

Sjur Brændeland

author
#0 lore raw

PATCH 1/4 stemodem: Add rtnl header file.

From: Sjur Brændeland <[email protected]>

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
drivers/stemodem/rtnl.h new file +41 -0
new file mode 100644
index 0000000..566452b
@@ -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);
+
Signature · 1 line
1.6.3.3
author
#1 lore raw

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

From: Sjur Brændeland <[email protected]>

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
drivers/stemodem/rtnl.c new file +365 -0
new file mode 100644
index 0000000..38f7f43
@@ -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);
+}
Signature · 1 line
1.6.3.3
#2 lore raw

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

Hi Sjur,

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 39 lines
drivers/stemodem/rtnl.c new file +39 -0
+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;

drivers/stemodem/rtnl.c line 320

Quoted 10 lines
drivers/stemodem/rtnl.c new file +10 -0
+	}
+
+	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

author
#3 lore raw

PATCH v2 1/2 stemodem: Add RTNL functionality managing CAIF Network Interfaces.

From: Sjur Brændeland <[email protected]>

---
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
Makefile.am +2 -0
index 2562160..fdd363d 100644
@@ -226,6 +226,8 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.c \
 			drivers/stemodem/voicecall.c \
 			drivers/stemodem/radio-settings.c \
+			drivers/stemodem/caif_rtnl.c \
+			drivers/stemodem/caif_rtnl.h \
 			drivers/stemodem/gprs-context.c \
 			drivers/stemodem/caif_socket.h \
 			drivers/stemodem/if_caif.h
drivers/stemodem/caif_rtnl.c new file +365 -0

365 lines not on this page · raw message

drivers/stemodem/caif_rtnl.h new file +40 -0
new file mode 100644
index 0000000..3d171f9
@@ -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);
Signature · 1 line
1.7.0.4
#4 lore raw

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

Hi Sjur,

Quoted 6 lines
 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.

drivers/stemodem/caif_rtnl.c line 41

Quoted 2 lines
drivers/stemodem/caif_rtnl.c new file +2 -0
+#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?

drivers/stemodem/caif_rtnl.c line 55

Quoted 13 lines
drivers/stemodem/caif_rtnl.c new file +13 -0
+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.

Quoted 6 lines
drivers/stemodem/caif_rtnl.c new file +6 -0
+static gboolean get_ifname(struct ifinfomsg *msg, int bytes,
+				const char **ifname)
+{
+	struct rtattr *attr;
+
+	for (attr = IFLA_RTA(msg); RTA_OK(attr, bytes);

drivers/stemodem/caif_rtnl.c line 72

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+			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 ;)

Quoted 20 lines
drivers/stemodem/caif_rtnl.c new file +20 -0
+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);

drivers/stemodem/caif_rtnl.c line 103

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+
+	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.

Quoted 91 lines
drivers/stemodem/caif_rtnl.c new file +91 -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;

drivers/stemodem/caif_rtnl.c line 205

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+	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.

Quoted 9 lines
drivers/stemodem/caif_rtnl.c new file +9 -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),

drivers/stemodem/caif_rtnl.c line 224

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+				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.

Quoted 21 lines
drivers/stemodem/caif_rtnl.c new file +21 -0
+	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;
+}

drivers/stemodem/caif_rtnl.c line 256

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+
+static gboolean netlink_event(GIOChannel *chan,
+				GIOCondition cond, gpointer data)
+{
+	unsigned char buf[4096];
+	gsize len;
+	GIOError err;
+
+	if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
+		return FALSE;

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

drivers/stemodem/caif_rtnl.c line 264

Quoted 8 lines
drivers/stemodem/caif_rtnl.c new file +8 -0
+
+	memset(buf, 0, sizeof(buf));
+
+	err = g_io_channel_read(chan, (gchar *) buf, sizeof(buf), &len);
+	if (err) {
+		if (err == G_IO_ERROR_AGAIN)
+			return TRUE;
+		return FALSE;

Same case here.

Quoted 21 lines
drivers/stemodem/caif_rtnl.c new file +21 -0
+	}
+
+	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));

drivers/stemodem/caif_rtnl.c line 295

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+	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.

Quoted 5 lines
drivers/stemodem/caif_rtnl.c new file +5 -0
+
+	return 0;
+}
+
+void caif_rtnl_exit(void)

drivers/stemodem/caif_rtnl.c line 310

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+{
+	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.

drivers/stemodem/caif_rtnl.c line 314

Quoted 4 lines
drivers/stemodem/caif_rtnl.c new file +4 -0
+	}
+
+	g_slist_free(pending_requests);
+	pending_requests = NULL;

No need to set pending_requests to NULL here.

drivers/stemodem/caif_rtnl.c line 319

Quoted 5 lines
drivers/stemodem/caif_rtnl.c new file +5 -0
+
+	g_io_channel_shutdown(channel, TRUE, NULL);
+	g_io_channel_unref(channel);
+
+	channel = NULL;

As described above, this is no longer needed.

drivers/stemodem/caif_rtnl.c line 325

Quoted 6 lines
drivers/stemodem/caif_rtnl.c new file +6 -0
+}
+
+
+int caif_rtnl_create_interface(struct rtnl_req_param *req_param)
+{
+	int ret;

Please use err as variable instead of ret.

drivers/stemodem/caif_rtnl.c line 335

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+	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;"

Quoted 4 lines
drivers/stemodem/caif_rtnl.c new file +4 -0
+	resp_param.user_data = req_param->user_data;
+
+	req->req = *req_param;
+	req->rsp = resp_param;

drivers/stemodem/caif_rtnl.c line 350

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+
+	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).

Review

Quoted 2 lines
(quoted diff) +2 -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.

drivers/stemodem/caif_rtnl.c line 363

Quoted 11 lines
drivers/stemodem/caif_rtnl.c new file +11 -0
+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.

Quoted 18 lines
drivers/stemodem/caif_rtnl.c new file +2 -0
+	return send_rtnl_req(&req);
+}
drivers/stemodem/caif_rtnl.h new file +10 -0
new file mode 100644
index 0000000..3d171f9
@@ -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.
+ *

drivers/stemodem/caif_rtnl.h line 20

Quoted 10 lines
drivers/stemodem/caif_rtnl.h new file +10 -0
+ *  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.

Quoted 7 lines
drivers/stemodem/caif_rtnl.h new file +7 -0
+struct rtnl_rsp_param {
+	int result;
+	gpointer user_data;
+	char ifname[IF_NAMESIZE];
+	int  ifindex;
+};
+

drivers/stemodem/caif_rtnl.h line 37

Quoted 10 lines
drivers/stemodem/caif_rtnl.h new file +10 -0
+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?

drivers/stemodem/caif_rtnl.h line 40

Quoted 3 lines
drivers/stemodem/caif_rtnl.h new file +3 -0
+
+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

#5 lore raw

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

Quoted 4 lines

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.

Quoted 6 lines
Quoted 4 lines
(quoted diff) +2 -0
+#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 ;-)

Quoted 22 lines
Quoted 7 lines
drivers/stemodem/caif_rtnl.c new file +6 -0
+static gboolean get_ifname(struct ifinfomsg *msg, int bytes,
+				const char **ifname)
+{
+	struct rtattr *attr;
+
+	for (attr = IFLA_RTA(msg); RTA_OK(attr, bytes);

drivers/stemodem/caif_rtnl.c line 72

Quoted 10 lines
drivers/stemodem/caif_rtnl.c new file +10 -0
+			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.

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 1 line

And here you should add an extra empty line.

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

Quoted 13 lines
Quoted 13 lines
drivers/stemodem/caif_rtnl.h new file +13 -0
+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;
Quoted 8 lines

drivers/stemodem/caif_rtnl.h line 37

Quoted 4 lines
drivers/stemodem/caif_rtnl.h new file +4 -0
+};
+
+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.

...

Quoted 4 lines

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

#6 lore raw

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

Hi Sjur,

Quoted 5 lines
Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
drivers/stemodem/caif_rtnl.h new file +13 -0
+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;
Quoted 8 lines

drivers/stemodem/caif_rtnl.h line 37

Quoted 4 lines
drivers/stemodem/caif_rtnl.h new file +4 -0
+};
+
+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?

Quoted 3 lines

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

#7 lore raw

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

Hi Marcel.

Quoted 10 lines

drivers/stemodem/caif_rtnl.c line 55

Quoted 4 lines
drivers/stemodem/caif_rtnl.c new file +4 -0
+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.

....

Quoted 2 lines

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.

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

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

Quoted 2 lines
Quoted 2 lines

anyway and we are a single threaded program. So the order is guaranteed.

...

Quoted 2 lines

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.

Quoted 4 lines

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);
Quoted 6 lines

drivers/stemodem/caif_rtnl.c line 314

Quoted 4 lines
drivers/stemodem/caif_rtnl.c new file +4 -0
+	}
+
+	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.

Quoted 11 lines
Quoted 1 line
Quoted 1 line

I am not really sold on this param stuct thingy btw. Why do you

need

Quoted 6 lines
Quoted 2 lines

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

#8 lore raw

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

Hi Sjur,

Quoted 10 lines
Quoted 10 lines

drivers/stemodem/caif_rtnl.c line 55

Quoted 4 lines
drivers/stemodem/caif_rtnl.c new file +4 -0
+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.

Quoted 6 lines

....

Quoted 2 lines

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?

Quoted 11 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

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

Quoted 2 lines
Quoted 2 lines

anyway and we are a single threaded program. So the order is guaranteed.

...

Quoted 2 lines

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.

Quoted 8 lines
Quoted 4 lines

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.

Quoted 9 lines
Quoted 6 lines

drivers/stemodem/caif_rtnl.c line 314

Quoted 4 lines
drivers/stemodem/caif_rtnl.c new file +4 -0
+	}
+
+	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 ;)

Quoted 16 lines
Quoted 11 lines
Quoted 1 line
Quoted 1 line

I am not really sold on this param stuct thingy btw. Why do you

need

Quoted 6 lines
Quoted 2 lines

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,

Quoted 7 lines
		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

author
#9 lore raw

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

In reply to Marcel Holtmann

Hi Marcel.

Quoted 3 lines

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

author
#10 lore raw

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

In reply to Marcel Holtmann

From: Sjur Brændeland <[email protected]>

---
 drivers/stemodem/gprs-context.c |  210 +++++++++++++++++++++++++-------------
 1 files changed, 138 insertions(+), 72 deletions(-)
drivers/stemodem/gprs-context.c +138 -72
index d3a50a9..e2b7e54 100644
@@ -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);
 }
Signature · 1 line
1.6.3.3
author
#11 lore raw

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

In reply to Sjur Brændeland

From: Sjur Brændeland <[email protected]>

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(-)
drivers/stemodem/gprs-context.c +138 -72
index d3a50a9..7e956d3 100644
@@ -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);
 }
Signature · 1 line
1.6.3.3
author
#12 lore raw

PATCH 4/4 stemodem: Add rtnl to Makefile.

In reply to Sjur Brændeland

From: Sjur Brændeland <[email protected]>

---
 Makefile.am |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Makefile.am +2 -0
index 2562160..1b09a3c 100644
@@ -226,6 +226,8 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.c \
 			drivers/stemodem/voicecall.c \
 			drivers/stemodem/radio-settings.c \
+			drivers/stemodem/rtnl.c \
+			drivers/stemodem/rtnl.h \
 			drivers/stemodem/gprs-context.c \
 			drivers/stemodem/caif_socket.h \
 			drivers/stemodem/if_caif.h
Signature · 1 line
1.6.3.3
#13 lore raw

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

Hi Sjur,

Quoted 5 lines

From: Sjur Brændeland <[email protected]>

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