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 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. > > > 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
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. 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,
> > 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
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,
> Makefile.am |2 +
> drivers/stemodem/caif_rtnl.c | 365
> ++
> drivers/stemodem/caif_rtnl.h | 40 +
> 3 files changed, 407 insertions(+), 0 deletions(-)
> create mode 100644 drivers/stemodem/caif_rtnl.c
> create mode 100644 drivers/stemodem/caif_rtnl.h
just to be clear. You want to name it like this? I am just asking here
since I made a few proposals.
> +#define NLMSG_TAIL(nmsg) \
> + ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len)))
We don't have this one as part of some kernel includes?
> +struct iplink_req {
> + struct nlmsghdr n;
> + struct ifinfomsg i;
> + char pad[1024];
> + int request_id;
> + struct rtnl_req_param req;
> + struct rtnl_rsp_param rsp;
> +};
> +
> +static GSList *pending_requests;
> +static GIOChannel *channel;
> +static guint32 rtnl_seqnr;
> +static guint rtnl_watch;
To be fair, you don't need both, the GIOChannel and the watch. You can
just add the watch and then unref the GIOChannel and set it to close on
unref. That way when the watch gets removed or you remove it, the
GIOChannel and the underlaying socket will be automatically closed as
well.
> +static gboolean get_ifname(struct ifinfomsg *msg, int bytes,
> + const char **ifname)
> +{
> + struct rtattr *attr;
> +
> + for (attr = IFLA_RTA(msg); RTA_OK(attr, bytes);
> + attr = RTA_NEXT(attr, bytes))
> +
> + if (attr->rta_type == IFLA_IFNAME &&
> + ifname != NULL) {
> + *ifname = RTA_DATA(attr);
> + return TRUE;
> + }
> +
> + return FALSE;
> +}
After I stared long enough at it, I realized that the code is fine, but
that was not obvious to me. In this case please use for () { } with the
curly braces around it. It improves readability.
And I know that other times we don't do { } for single statements, but
here is does helps my poor human brain ;)
> +static void store_newlink_param(struct iplink_req *req, unsigned short type,
> + int index, unsigned flags,
> + unsigned change, struct ifinfomsg *msg,
> + int bytes)
> +{
> + const char *ifname = NULL;
> +
> + get_ifname(msg, bytes, &ifname);
> + strncpy(req->rsp.ifname, ifname,
> + sizeof(req->rsp.ifname));
> + req->rsp.ifname[sizeof(req->rsp.ifname)-1] = '\0';
> + req->rsp.ifindex = index;
> +}
> +
> +static int send_rtnl_req(struct iplink_req *req)
> +{
> + struct sockaddr_nl addr;
> + int sk, ret;
> +
> + sk = g_io_channel_unix_get_fd(channel);
> +
> + memset(&addr, 0, sizeof(addr));
> + addr.nl_family = AF_NETLINK;
> +
> + ret = sendto(sk, req, req->n.nlmsg_len, 0,
> + (struct sockaddr *) &addr, sizeof(addr));
> + if (ret < 0)
> + return ret;
> + return 0;
> +}
Can we just not do "return sendto(..." and then have the caller to a
proper "if send_rtnl_req() < 0)" check. Looks much simpler to me.
> +static struct iplink_req *find_request(guint32 seq)
> +{
> + GSList *list;
> +
> + for (list = pending_requests; list; list = list->next) {
> + struct iplink_req *req = list->data;
> +
> + if (req->n.nlmsg_seq == seq)
> + return req;
> + }
> +
> + return NULL;
> +}
> +
> +static void rtnl_client_response(struct iplink_req *req)
> +{
> +
> + if (req->req.callback && req->n.nlmsg_type == RTM_NEWLINK)
> + req->req.callback(&req->rsp);
> +
> + pending_requests = g_slist_remove(pending_requests, req);
> + g_free(req);
> +}
> +
> +static void parse_rtnl_message(void *buf, size_t len)
> +{
> + struct ifinfomsg *msg;
> + struct iplink_req *req;
> +
> + while (len > 0) {
> + struct nlmsghdr *hdr = buf;
> + if (!NLMSG_OK(hdr, len))
> + break;
> + if (hdr->nlmsg_type == RTM_NEWLINK) {
> + req = g_slist_nth_data(pending_requests, 0);
> + DBG("NEWLINK req:%p\n", req);
> + if (req == NULL)
> + break;
> + msg = (struct ifinfomsg *) NLMSG_DATA(hdr);
> + store_newlink_param(req, msg->ifi_type,
> + msg->ifi_index, msg->ifi_flags,
> + msg->ifi_change, msg,
> + IFA_PAYLOAD(hdr));
> + rtnl_client_response(req);
> + return;
> +
> + } else if (hdr->nlmsg_type == NLMSG_ERROR) {
> + req = find_request(hdr->nlmsg_seq);
> + DBG("NLMSG ERROR req:%p\n", req);
> + if (req == NULL)
> + break;
> +
