Re: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
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 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
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 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
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 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
From: Sjur Brændeland sjur.brandel...@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 000..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) +{
RE: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
From: Marcel Holtmann [mailto:mar...@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 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
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 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono