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;
> +
[PATCHv2 2/2] stemodem: Update gprs-context to use rtnl to create/remove interfaces.
From: Sjur Brændeland
---
drivers/stemodem/gprs-context.c | 210 +-
1 files changed, 138 insertions(+), 72 deletions(-)
diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index d3a50a9..e2b7e54 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
@@ -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);
[PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
From: Sjur Brændeland
---
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
+#endif
+
+#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)))
+
+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)
File Structure for persisting history in a disk file (Comments please)
Hello, I want to use the following file structure for persisting history in a disk file. Can you please send me your feedback about it. Thanks Raji Bommaraju _File structure for History Persistance_ History information will be stored in a disk file. The file will have a File Header consisting of "Bytes Stored" and "Head" for writing the next record.Each record stored will have record header and record data, record header has "record type" and "size of the record" (For Text messages the records will have variable length message data resulting in variable length records) hence this record structure is used. *File Format:* |File Header| Data | File Header (8 bytes): |Bytes stored | Head| 048 *Data:* |Record|Record|.etc *Record:* |Record Header|Actual data| *Record Header:* Record Type: 1byte (voice call history - 0, text history- 1) Size : Integer Actual data: Will be voice call or text message information ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 2/4] stemodem: Add RTNL functionality for CAIF Netw Interface.
Hi Sjur,
> 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
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.
And I have not looked through the whole patch yet.
> +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;
> +}
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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 4/4] stemodem: Add rtnl to Makefile.
Hi Sjur, > From: Sjur Brændeland > > --- > 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 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] TODO: update owner of see/cancel pending SMS task
Hi Yang, > TODO |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) patch has been applied. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 2/2] main: add capabilities for phonet
Hi Mika, > Phonet sockets require CAP_SYS_ADMIN and SO_BINDTODEVICE socket > option requires CAP_NET_RAW. and I so thought that we get away with lower capabilities :( > --- > src/main.c |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) Patch has been applied. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 1/2] isigen: fix phonet address initialization
Hi Mika, > plugins/isigen.c |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) patch has been applied. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 0/2] phonet initialization fixes
Hi All, A couple of small fixes for phonet. Br, MikaL [PATCH 1/2] isigen: fix phonet address initialization [PATCH 2/2] main: add capabilities for phonet plugins/isigen.c |3 ++- src/main.c |3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 2/2] main: add capabilities for phonet
Phonet sockets require CAP_SYS_ADMIN and SO_BINDTODEVICE socket option requires CAP_NET_RAW. --- src/main.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/main.c b/src/main.c index 93149bc..eca008e 100644 --- a/src/main.c +++ b/src/main.c @@ -140,7 +140,8 @@ int main(int argc, char **argv) /* Drop capabilities */ capng_clear(CAPNG_SELECT_BOTH); capng_updatev(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, - CAP_NET_BIND_SERVICE, CAP_NET_ADMIN, -1); + CAP_NET_BIND_SERVICE, CAP_NET_ADMIN, + CAP_NET_RAW, CAP_SYS_ADMIN, -1); capng_apply(CAPNG_SELECT_BOTH); #endif -- 1.7.0.4 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 1/2] isigen: fix phonet address initialization
---
plugins/isigen.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/plugins/isigen.c b/plugins/isigen.c
index 0ea2db3..493d926 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -286,9 +286,10 @@ static int isigen_probe(struct ofono_modem *modem)
}
if (address) {
- int error = g_pn_netlink_set_address(idx, PN_DEV_PC);
+ int error = g_pn_netlink_set_address(idx, address);
if (error && error != -EEXIST) {
DBG("g_pn_netlink_set_address: %s\n", strerror(-error));
+ g_pn_netlink_stop(link);
return -errno;
}
}
--
1.7.0.4
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH 2/3] voicecall: emergency call handling added
Hi Denis, On 10/29/2010 08:32 PM, ext Denis Kenzior wrote: Hi Andras, Just some general comments, but this patch seems to be backwards from the earlier proposal. Namely EmergencyMode is a property on the Modem interface, not on the VoiceCallManager. See doc/modem-api.txt, Emergency property. I thought it was more logical to have the EmergencyMode property linked to voicecall since it is about a special call case, but I am fine with moving that property up to modem level. Oh I can see that as well, but I think earlier we agreed that it should be on the Modem interface. This has several advantages: offline / online toggling has to happen in the modem and it is easier for the power management framework to monitor it there. So unless you feel really strongly against that I suggest we stick with the earlier proposal. I see the advantage of using the Modem interface and I am fine with basing the implementation on it. In general I think that the emergency_watch is unnecessary. Having a reference counted emergency tracking inside the modem object and a modem online state watch should be sufficient. The idea with the emergency watch is that any subsystem can get the notification when the emergency mode is entered and react on it. Don't get me wrong, it might be useful in the future. But for the context of supporting emergency calls in the voicecall driver the emergency watch is not really needed. In general I prefer to review code which has an immediate or foreseeable need. In this case if we detect an emergency call dial and we're offline, we: - Save the pending call - establish an online watch - ofono_modem_inc_emergency_mode() Once we are online: - Dial the call Once the call ends - ofono_modem_dec_emergency_mode() Nowhere do we actually need to use an emergency watch itself. To give you a more complex example, it might well be that the gprs connection needs to be torn down when making an emergency call in 2G mode, there are such networks out there that prevents you from making an emergency call if your device is attached to a PDP context. In this given situation it comes to the question how to bring down the gprs connection. It can be done such that the gprs atom will tear down the connection after receiving the EmergencyMode notification, or another option is to have gprs connection handling functions made Have we established that this is actually still needed? I thought you guys said all the networks that have this problem have been fixed by now? If this is still required, I suggest you group the emergency watch functions with patches implementing the above functionality. available by gprs and to deal with the gprs connection within voicecall (or somewhere else). The online/offline mode change handling in fact is bringing up the some issue, how the mode change handling should be implemented when making the emergency call. My idea was let every subsystem deal with the specifics of its own subsystem. Let the modem figure out the specifics. Basically as long as the count for emergency is greater than 1, Offline mode should not be entered. Once it reaches 0, the online mode should go back to the previous value. Based on the comments and after some clarifications made in our team, I consider the emergency watch unnecessary. We can forget about the very corner case and go with the simpler approach. OK, I am going to come up with a new set of patches. Regards, -Denis Regards, Andras ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC PATCH 2/3] voicecall: emergency call handling added
Hi Marcel, > > > Have we established that this is actually still needed? > I thought you > > > guys said all the networks that have this problem have been > > > fixed by now? > > > > AFAIK, this is still a product requirement for us. I can > recheck, but I just asked last week and the answer was a very > definite "Yes, this is still required". > > as Denis and I mentioned, we have been told that this is not needed > anymore. So where is this still needed? Ok, I double checked the product requirements and it turns out that you're right. The requirement is no longer there for oFono based stuff. Seems we've had some wires crossed at our end, so the situation was not entirely clear. Sorry for the confusion, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC PATCH 2/3] voicecall: emergency call handling added
Hi Mika, > > > To give you a more complex example, it might well be that the gprs > > > connection needs to be torn down when making an emergency call in > > > 2G mode, there are such networks out there that prevents you from > > > making an emergency call if your device is attached to a > > PDP context. > > > In this given situation it comes to the question how to > > bring down the > > > gprs connection. It can be done such that the gprs atom > > will tear down > > > the connection after receiving the EmergencyMode notification, or > > > another option is to have gprs connection handling functions made > > > > Have we established that this is actually still needed? I thought you > > guys said all the networks that have this problem have been > > fixed by now? > > AFAIK, this is still a product requirement for us. I can recheck, but I just > asked last week and the answer was a very definite "Yes, this is still > required". as Denis and I mentioned, we have been told that this is not needed anymore. So where is this still needed? And again, this is something that the modem firmware should be doing anyway (if it is needed). The modem firmware knows best. And we do have the GPRS suspend support already to signal this back to the user. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/6] radio settings: add FastDormancy property
Hi Denis, > One of the biggest principles in oFono is not to perform premature > optimization. Sure there is a potential issue, but nobody > knows whether > it will actually manifest itself outside of malicious code (which we > tell to bugger off) or what the most common manifestation > pattern will be. > > If / once we know for sure this is a problem, then we can solve it > properly. So far this approach has been working very nicely for us. > There are countless occasions where taking the wait-and-see > approach and > gathering more information allowed us to devise a much better solution > than we would have originally. > > So the general rule is: Do the simplest thing that is likely to work. > If it doesn't work, improve it. Can't fault the approach, it's generally a good one. That said, I see this more as an API quality issue rather than an optimization issue. Anyway, I've raised the concern. Let's hope my worries are unfounded. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] TODO: update owner of see/cancel pending SMS task
--- TODO |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/TODO b/TODO index a01960e..5968c08 100644 --- a/TODO +++ b/TODO @@ -70,6 +70,7 @@ SMS Priority: High Complexity: C2 + Owner: Yang Gu - Persist outgoing SMS messages. Currently oFono persists incoming messages that are fragmented. However oFono does not persist queued outgoing -- 1.7.2.3 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC PATCH 2/3] voicecall: emergency call handling added
Hi, > > To give you a more complex example, it might well be that the gprs > > connection needs to be torn down when making an emergency call in > > 2G mode, there are such networks out there that prevents you from > > making an emergency call if your device is attached to a > PDP context. > > In this given situation it comes to the question how to > bring down the > > gprs connection. It can be done such that the gprs atom > will tear down > > the connection after receiving the EmergencyMode notification, or > > another option is to have gprs connection handling functions made > > Have we established that this is actually still needed? I thought you > guys said all the networks that have this problem have been > fixed by now? AFAIK, this is still a product requirement for us. I can recheck, but I just asked last week and the answer was a very definite "Yes, this is still required". MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
