RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Marcel. Does anything cleans up the channel when oFono unexpectedly crashes? No not yet. I suppose the interface will continue up until someone destroys it. Should we simply destroy all CAIF interfaces if oFono crashes? I think this would be the simplest approach. BTW, how does connman behave in this case? Will it do any attempt to clean up IP interfaces when it notices that oFono APIs has gone missing? And what about modem state, is it taken back to flight-mode (AT+CFUN=0) in order to reset everything? If you don't mind, I would prefer to do restart handling as the next step. It would be nice to finish this patch review first, what do you think? I think you can just use a stack buffer when you actually send the message. This is damn ugly and from a security point it actually scares me. Please only keep the seqnum in here. Since that is the only thing you need to find that pending request. Just get rid of everything that you don't need for the callback. Yes, done. I have squashed this into parse_rtnl_message, as you suggested. I'm not really convinced this was any cleaner though..? I end up with duplicating the code in the two if statements. We will see. I might have been wrong, but in my mind it just locked fine. I will take a second look when you send the updated patch. OK, tell me what you think about the next patch then. I think that the kernel implementation of rtnl will guarantee the requests to be processed in order. rtnetlink_rcv in ..net/core/rtnetlink.c takes the rtnl_lock before processing the netlink messages. I believe this will guarantee that the NEWLINK responses will be received in the same order as they are sent. it might be keeping the order, but I wouldn't count on it. And just using your find request function would make this code simpler. The seqnr is not present in the message header of this response message, so this will not be possible. I would prefer to stick with the current implementation. Or do you see another way forward? + req = find_request(hdr-nlmsg_seq); + if (req == NULL) + break; + + DBG(nlmsg error req); + rtnl_client_response(req, -1); + return; + } If I understand this right, then you can always find the pending request and remove it here. No need for rtnl_client_response at all. Yes, I guess you are right. I can remove this callback. Currently I am only actually interested in the result when the interface is created successfully. Nothing particularly is done in gprs-context.c if it fails. That is not what I meant. We should find that request and call the callback, but I think this can be done a more generic way. So whatever message you get, find the request that it belongs to. You need that request struct anyway. And then just reply here. OK, I think I have done so. Have a look when I send the next patch. Actually I think io_channel_read is just fine here. I think connman is doing the same thing for rtnl. It is the sending part which is problematic. We should not be using that in ConnMan either actually. The _read is fine, but deprecated. The new _read_chars is doing buffering and a bit nasty in that area. So really just use recv() here. OK, I've changed it to recv. So if we split out creating the NLMSG header into one helper functions, then you can easily prep the RTNL newlink message here inside the function. Even with creating this helper function you suggested the prep_rtnl_newlink_req is on 45 lines. I think it makes sense still keep then separate. It is no big deal for me, but I would prefer to avoid such a large functions, As I said above, it looked good in my head. Maybe I was wrong, but in general this is pretty simple sequential stuff. So it can be in one function and still be easy readable. Lets have a look on how it looks and then decide. OK, I've squashed the two functions. This will also allow you to remove req-loop_enabled variables since I don't see need for storing them. You are right, the loop_enabled variable is not currently needed. However for various testing purposes we frequently want to run the CAIF Interface in loopback mode (e.g. testing loopback to the modem). loopback_mode will make the interface swap src and destination ip addresses so we actually can do ping etc. If you are OK with this, I would prefer to keep this in order to make testing easier. But again, I'm easy... Having loopback mode is fine. I just meant that you don't have to store the variable in the request struct since you build the request already. Please store only things in your request struct that you do need later on. OK, done. This struct is much smaller now. diff --git a/drivers/stemodem/caif_rtnl.h ... +typedef void (*caif_rtnl_create_cb_t)
RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Marcel.
what happens for network triggered deactivation. Some networks here
disconnect the GPRS context used MMS. Has some funny behavior that need
to be taken into account.
Good question! When code reading with this in mind I realize that
we missed to mark the conn_info-interface as available by setting
cid = 0. This is bug, thank you for leading me to it.
FYI this is how CAIF and AT works together:
In order to have payload over CAIF there need to be
both a CAIF channel connected and a context activated with AT*EPPSD.
When Channel-ID configured for CAIF IP Interface and the Channel-Id given
in AT*EPPSD matches, the modem side will connect the CAIF Channel to
the Network Signaling Stack in the modem.
With this new patch CAIF channels are created statically from probe.
Activation and deactivation is controlled by AT*EPPSD, or notified by
+CGEV which will result in a CGACT status query.
If the network disconnects GPRS we should receive a
+CGEV: NW DEACT, subsequent CGACT status query.
Here the connection (and CAIF Network Interface) should be
marked as unused by setting cid to zero.
+#define NLMSG_TAIL(nmsg) \
+ ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-
nlmsg_len)))
We have no global define this? You wanted to look into that. What was
the outcome of it?
I did mention this before - I have looked around, but he did not find
any relevant macro for this unfortunately.
+struct iplink_req {
+ struct nlmsghdr n;
+ struct ifinfomsg i;
+ char pad[1024];
What is this huge pad for? It looks totally fishy to me.
The pad is there to hold the remaining of the rtnl attributes.
I'm working on restructuring this so that I can separate the
buffer holding the rtnl_message and the iplink_req.
I just have to make it work...
+static guint rtnl_watch;
Get rid of the double spaces for rtnl_watch.
+static GIOChannel *channel;
If we follow your convention here and not to overload variables this
might be better named rtnl_channel.
And yes, I am fine with keeping it since there is no way to connect a
netlink socket properly to just use send(). You will require to use
sendto for everything.
OK, Done.
+static void store_newlink_param(struct iplink_req *req, unsigned
short type,
+ int index, unsigned flags,
+ unsigned change, struct ifinfomsg *msg,
+ int bytes)
+{
+ const char *ifname = NULL;
+
+ get_ifname(msg, bytes, ifname);
+ strncpy(req-ifname, ifname,
+ sizeof(req-ifname));
+ req-ifname[sizeof(req-ifname)-1] = '\0';
+ req-ifindex = index;
+}
So I think that store_newlink_... and get_ifname can be nicely combined
into one helper function. And using extract_parameters() function name
is a bit better.
And please only add parameters to that function that you really need in
there. And the iplink_req should be last parameter.
As one general is to have input parameters first and then the output
parameters last. So read this something like extract values from this
structure to this structure.
Yes, I agree. I have squashed the two functions together and cleaned up the
parameters.
This is much nicer, thank you.
+static int send_rtnl_req(struct iplink_req *req)
+{
+ struct sockaddr_nl addr;
+ int sk = g_io_channel_unix_get_fd(channel);
+
+ memset(addr, 0, sizeof(addr));
+ addr.nl_family = AF_NETLINK;
+
+ return sendto(sk, req, req-n.nlmsg_len, 0,
+ (struct sockaddr *) addr, sizeof(addr));
+}
I don't think this should be a separate function. You use it only twice
and having it close to the code using it would be better.
OK, good I have squashed these to functions as well.
+static struct iplink_req *find_request(guint32 seq)
+{
+ GSList *list;
+
+ for (list = pending_requests; list; list = list-next) {
+ struct iplink_req *req = list-data;
+
+ if (req-n.nlmsg_seq == seq)
+ return req;
+ }
+
+ return NULL;
+}
I would move this function up in this file at the top. It should go
close to the variable declaration for pending_request.
OK, done.
+static void rtnl_client_response(struct iplink_req *req, int result)
+{
+
+ if (req-callback req-n.nlmsg_type == RTM_NEWLINK)
+ req-callback(result, req-user_data,
+ req-ifname, req-ifindex);
+
+ pending_requests = g_slist_remove(pending_requests, req);
+ g_free(req);
+}
I don't like this split out. You already checked the nlmsg_type and
here
you keep checking it again just to reuse some code. This looks like
waste to me.
I have squashed this into parse_rtnl_message, as you suggested.
I'm not really convinced this was any cleaner though..?
I end up with duplicating the code in the two if statements.
+static void parse_rtnl_message(void *buf, size_t len)
RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Sjur,
what happens for network triggered deactivation. Some networks here
disconnect the GPRS context used MMS. Has some funny behavior that need
to be taken into account.
Good question! When code reading with this in mind I realize that
we missed to mark the conn_info-interface as available by setting
cid = 0. This is bug, thank you for leading me to it.
fall into a similar trap with GPRS support for IFX.
FYI this is how CAIF and AT works together:
In order to have payload over CAIF there need to be
both a CAIF channel connected and a context activated with AT*EPPSD.
When Channel-ID configured for CAIF IP Interface and the Channel-Id given
in AT*EPPSD matches, the modem side will connect the CAIF Channel to
the Network Signaling Stack in the modem.
With this new patch CAIF channels are created statically from probe.
Activation and deactivation is controlled by AT*EPPSD, or notified by
+CGEV which will result in a CGACT status query.
If the network disconnects GPRS we should receive a
+CGEV: NW DEACT, subsequent CGACT status query.
Here the connection (and CAIF Network Interface) should be
marked as unused by setting cid to zero.
Sounds good to me.
Does anything cleans up the channel when oFono unexpectedly crashes?
+#define NLMSG_TAIL(nmsg) \
+ ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-
nlmsg_len)))
We have no global define this? You wanted to look into that. What was
the outcome of it?
I did mention this before - I have looked around, but he did not find
any relevant macro for this unfortunately.
I might just overread your response. Just wanted to make sure.
+struct iplink_req {
+ struct nlmsghdr n;
+ struct ifinfomsg i;
+ char pad[1024];
What is this huge pad for? It looks totally fishy to me.
The pad is there to hold the remaining of the rtnl attributes.
I'm working on restructuring this so that I can separate the
buffer holding the rtnl_message and the iplink_req.
I just have to make it work...
I think you can just use a stack buffer when you actually send the
message. This is damn ugly and from a security point it actually scares
me.
Please only keep the seqnum in here. Since that is the only thing you
need to find that pending request. Just get rid of everything that you
don't need for the callback.
+static void rtnl_client_response(struct iplink_req *req, int result)
+{
+
+ if (req-callback req-n.nlmsg_type == RTM_NEWLINK)
+ req-callback(result, req-user_data,
+ req-ifname, req-ifindex);
+
+ pending_requests = g_slist_remove(pending_requests, req);
+ g_free(req);
+}
I don't like this split out. You already checked the nlmsg_type and
here
you keep checking it again just to reuse some code. This looks like
waste to me.
I have squashed this into parse_rtnl_message, as you suggested.
I'm not really convinced this was any cleaner though..?
I end up with duplicating the code in the two if statements.
We will see. I might have been wrong, but in my mind it just locked
fine. I will take a second look when you send the updated patch.
An you can have more than one RTNL message inside this buffer. You need
to be able to handle this. Otherwise you might loose responses. The
case
of just calling return when you received one of the two messages you
care about is not really acceptable.
OK, done. I'll keep looping.
+ while (len 0) {
+ struct nlmsghdr *hdr = buf;
+
+ if (!NLMSG_OK(hdr, len))
+ break;
+
+ if (hdr-nlmsg_type == RTM_NEWLINK) {
+ req = g_slist_nth_data(pending_requests, 0);
+ if (req == NULL)
+ break;
How does this work? You just pick the first pending request and don't
really compare the sequence number. Who guarantees the order of these
events. I don't think we should be doing this.
I think that the kernel implementation of rtnl will guarantee the
requests to be processed in order. rtnetlink_rcv in ..net/core/rtnetlink.c
takes the rtnl_lock before processing the netlink messages.
I believe this will guarantee that the NEWLINK responses will be received
in the same order as they are sent.
it might be keeping the order, but I wouldn't count on it. And just
using your find request function would make this code simpler.
+ msg = (struct ifinfomsg *) NLMSG_DATA(hdr);
+ store_newlink_param(req, msg-ifi_type,
+ msg-ifi_index, msg-ifi_flags,
+ msg-ifi_change, msg,
+ IFA_PAYLOAD(hdr));
+ rtnl_client_response(req, 0);
+ return;
+
+ } else if (hdr-nlmsg_type == NLMSG_ERROR) {
So I would prefer if you use a switch statement here. It
[PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
From: Sjur Brændeland sjur.brandel...@stericsson.com
---
Hi Marcel.
I'm sorry about the formatting for the v3 version of this patch.
I used git send-email via my gmail account, and ended up with base64
MIME content encoding. I dont' know what went wrong :-(
I think I have closed most of your review comments so far. I kept using
sendto as I don't quite get how to use g_io_channel_write_chars for rtnl socket.
(I think connman is using sendto as well.) I still set g_caif_devices = NULL
just in
case someone in future does init/exit more than once.
The patch has been tested activating/deactivation and I have run valgrind
showing
no leaks on some unit tests (not yet upstreamed).
Regards,
Sjur
Makefile.am |2 +
drivers/stemodem/caif_rtnl.c | 379 ++
drivers/stemodem/caif_rtnl.h | 29
3 files changed, 410 insertions(+), 0 deletions(-)
create mode 100644 drivers/stemodem/caif_rtnl.c
create mode 100644 drivers/stemodem/caif_rtnl.h
diff --git a/Makefile.am b/Makefile.am
index 05082de..f163b0a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -226,6 +226,8 @@ builtin_sources += drivers/atmodem/atutil.h \
drivers/stemodem/stemodem.c \
drivers/stemodem/voicecall.c \
drivers/stemodem/radio-settings.c \
+ drivers/stemodem/caif_rtnl.c \
+ drivers/stemodem/caif_rtnl.h \
drivers/stemodem/gprs-context.c \
drivers/stemodem/caif_socket.h \
drivers/stemodem/if_caif.h
diff --git a/drivers/stemodem/caif_rtnl.c b/drivers/stemodem/caif_rtnl.c
new file mode 100644
index 000..ad58c93
--- /dev/null
+++ b/drivers/stemodem/caif_rtnl.c
@@ -0,0 +1,379 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2010 ST-Ericsson AB.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include config.h
+#endif
+
+#include string.h
+#include unistd.h
+#include errno.h
+#include net/if.h
+#include net/if_arp.h
+#include fcntl.h
+#include linux/rtnetlink.h
+
+#include glib.h
+
+#include ofono/log.h
+
+#include if_caif.h
+#include caif_rtnl.h
+
+#define NLMSG_TAIL(nmsg) \
+ ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-nlmsg_len)))
+
+struct iplink_req {
+ struct nlmsghdr n;
+ struct ifinfomsg i;
+ char pad[1024];
+
+ int request_id;
+ int type;
+ int conn_id;
+ gpointer user_data;
+ gboolean loop_enabled;
+
+ char ifname[IF_NAMESIZE];
+ int ifindex;
+ caif_rtnl_create_cb_t callback;
+};
+
+static GSList *pending_requests;
+static guint32 rtnl_seqnr;
+static guint rtnl_watch;
+static GIOChannel *channel;
+
+static gboolean get_ifname(struct ifinfomsg *msg, int bytes,
+ const char **ifname)
+{
+ struct rtattr *attr;
+
+ for (attr = IFLA_RTA(msg); RTA_OK(attr, bytes);
+ attr = RTA_NEXT(attr, bytes)) {
+
+ if (attr-rta_type == IFLA_IFNAME
+ ifname != NULL) {
+
+ *ifname = RTA_DATA(attr);
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
+
+static void store_newlink_param(struct iplink_req *req, unsigned short type,
+ int index, unsigned flags,
+ unsigned change, struct ifinfomsg *msg,
+ int bytes)
+{
+ const char *ifname = NULL;
+
+ get_ifname(msg, bytes, ifname);
+ strncpy(req-ifname, ifname,
+ sizeof(req-ifname));
+ req-ifname[sizeof(req-ifname)-1] = '\0';
+ req-ifindex = index;
+}
+
+static int send_rtnl_req(struct iplink_req *req)
+{
+ struct sockaddr_nl addr;
+ int sk = g_io_channel_unix_get_fd(channel);
+
+ memset(addr, 0, sizeof(addr));
+ addr.nl_family = AF_NETLINK;
+
+ return sendto(sk, req, req-n.nlmsg_len, 0,
+ (struct sockaddr *) addr, sizeof(addr));
+}
+
+static struct iplink_req *find_request(guint32 seq)
+{
+ GSList *list;
+
+ for (list = pending_requests; list; list = list-next) {
+ struct iplink_req *req = list-data;
+
+
