Re: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Sjur,
> 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).
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.
> 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
> +#endif
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +
> +#include "if_caif.h"
> +#include "caif_rtnl.h"
> +
> +#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?
> +struct iplink_req {
> + struct nlmsghdr n;
> + struct ifinfomsg i;
> + char pad[1024];
What is this huge pad for? It looks totally fishy to me.
> +
> + 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;
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.
> +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;
> +}
Any reason why not just return const char * and take NULL as 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(ms
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 suggest
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));
> > > +
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
