Re: [PATCH v5b 2/3] stemodem: Create network interfaces statically

2010-11-12 Thread Marcel Holtmann
Hi Sjur,

>  static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
> @@ -191,7 +189,7 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult 
> *result,
>   ofono_gprs_context_cb_t cb = cbd->cb;
>   struct ofono_gprs_context *gc = cbd->user;
>   struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> - struct conn_info *conn;
> + struct conn_info *conn = NULL;
>   GSList *l;

why do you need the conn = NULL assignment here? I haven't even looked
into your code flow, but I prefer clearly to not have these.

Regards

Marcel


___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


Re: [PATCH v5b 1/3] stemodem: Fix for error handling, memleak and changed some defines

2010-11-12 Thread Marcel Holtmann
Hi Sjur,

> * renamed MAX_LEN to IP_ADDR_LEN
> * removed memory leak from unneeded strdup when parsing xml response.
> * better handling of AT error responses
> * reduced number of caif interfaces to 4
> ---
>  drivers/stemodem/gprs-context.c |   54 ++
>  1 files changed, 31 insertions(+), 23 deletions(-)

patch has been applied. Thanks.

Regards

Marcel


___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


Re: [PATCH v5] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

2010-11-12 Thread Marcel Holtmann
Hi Sjur,

>  Makefile.am  |2 +
>  drivers/stemodem/caif_rtnl.c |  340 
> ++
>  drivers/stemodem/caif_rtnl.h |   29 
>  3 files changed, 371 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/stemodem/caif_rtnl.c
>  create mode 100644 drivers/stemodem/caif_rtnl.h

I applied this patch now, but I had to fix this up a bit. You have to
send a patch that updates this one.

> +#define RTNL_MSG_SIZE 4096
> +
> +struct rtnl_msg {
> + struct nlmsghdr n;
> + struct ifinfomsg i;
> + char data[RTNL_MSG_SIZE];
> +};

Is this not a bit big?

> +struct iplink_req {
> + guint32 rtnlmsg_seqnr;

Use the proper nlmsg_seqnr type that you get from RTNL.

> + gpointer user_data;

Use void * here since your callback does as well.

> + caif_rtnl_create_cb_t callback;
> +};
> +
> +static GSList *pending_requests;
> +static guint32 rtnl_seqnr;
> +static guint rtnl_watch;
> +static GIOChannel *rtnl_channel;
> +
> +static struct iplink_req *find_request(guint32 seq)
> +{

seq should match RTNL given type.

> + GSList *list;
> +
> + for (list = pending_requests; list; list = list->next) {
> + struct iplink_req *req = list->data;
> +
> + if (req->rtnlmsg_seqnr == seq)
> + return req;
> + }
> +
> + return NULL;
> +}
> +
> +static void parse_newlink_param(struct ifinfomsg *msg, int size,
> + int *ifindex, char *ifname)
> +{
> + struct rtattr *attr;
> +
> + for (attr = IFLA_RTA(msg); RTA_OK(attr, size);
> + attr = RTA_NEXT(attr, size)) {
> +
> + if (attr->rta_type == IFLA_IFNAME &&
> + ifname != NULL) {
> +
> + strncpy(ifname, RTA_DATA(attr), IF_NAMESIZE);
> + ifname[IF_NAMESIZE-1] = '\0';
> + break;
> + }
> + }
> +
> + *ifindex = msg->ifi_index;
> +}
> +
> +static void parse_rtnl_message(const void *buf, size_t len)
> +{
> + struct ifinfomsg *msg;
> + struct iplink_req *req = NULL;

This is bad. Please only do assignment if there is no other way around
it. I fixed this for you.

Regards

Marcel


___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


Re: [RfC] SIM file watch support.

2010-11-12 Thread Andrzej Zaborowski
Hi,

On 11 November 2010 16:50, Denis Kenzior  wrote:
> On 11/05/2010 07:13 AM, Andrzej Zaborowski wrote:
>> In order to support the Refresh STK command we need to implement at
>> least two types of reset: UICC reset (manufacturer specific) and NAA
>> application reset.  Other types can fall back to application reset
>> which can be implemented by removing all atoms and reinitialising them.

[...]

>> stk.c will check if any of the file IDs supplied in the command have
>> any watches on them.  If there are only watches with non-NULL
>> callback, it'll call all of them and not reset application.  If
>> there are any NULL notifiers, then stk.c will fall back to full
>> application reset.  If any of the files were cached, they need to
>> be re-read.
>
> So the last paragraph is confusing me a bit.  What is meant by a full
> application reset?  Do you mean simulating a sim removal and
> re-insertion as per a UICC reset?

Yes, whatever will cause rerunning sim_initialize.  In terms of the
SIM state state machine I'm not sure if ofono should go back only to
_INSERTED state (in case it was in _READY), or to _NOT_PRESENT and
then immediately _INSERTED.

It looks like some atoms (like cbs) need to start watching SIM state
to find out about imsi changes because they use it to initialise
storage.

Also I'm wondering if we need to clear all of the SIM cache in case of
a Refresh with "UICC reset" or "NAA initialisation", or if we can
assume that the card will first issue a Refresh with File Change
Notification every time a file changes (I'm thinking about the tricky
files like EFust)

>
>> ---
>>  src/simfs.h |   12 
>>  src/sim.c   |   21 -
>>  2 files changed, 32 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/simfs.h b/src/simfs.h
>> index ef962db..679955a 100644
>> --- a/src/simfs.h
>> +++ b/src/simfs.h
>> @@ -25,6 +25,8 @@ typedef void (*sim_fs_read_info_cb_t)(int ok, unsigned 
>> char file_status,
>>                                       int total_length, int record_length,
>>                                       void *userdata);
>>
>> +typedef void (*sim_fs_ef_notify_t)(int id, void *userdata);
>> +
>>  struct sim_fs *sim_fs_new(struct ofono_sim *sim,
>>                               const struct ofono_sim_driver *driver);
>>
>> @@ -48,3 +50,13 @@ char *sim_fs_get_cached_image(struct sim_fs *fs, int id);
>>  void sim_fs_cache_image(struct sim_fs *fs, const char *image, int id);
>>
>>  void sim_fs_free(struct sim_fs *fs);
>> +
>> +int sim_fs_add_ef_watch(struct sim_fs *fs, int id,
>> +                     sim_fs_ef_notify_t *notify, void *userdata);
>> +
>> +int sim_fs_remove_ef_watch(struct sim_fs *fs, int id,
>> +                     sim_fs_ef_notify_t *notify, void *userdata);
>> +
>> +ofono_bool_t sim_fs_has_empty_watches(struct sim_fs *fs, int id);
>
> Shouldn't we be adding the watch functionality to the STK atom?  I think
> logically it belongs there.  Otherwise the API seems fine to me.
>
>> +
>> +int sim_fs_notify_file_change(struct sim_fs *fs, int id);
>
> This actually sounds fine.  Do you think this function needs to also
> peek inside the currently running queue and terminate the reading of
> files that are changed?

Yes, both are good ideas.  While I think the file watch thing
logically is better in simfs.h, other atoms can't access it there so
it needs to be a SIM atom api.

Best regards
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


[RFC 3/3] phonesim: Add modem reset trigger

2010-11-12 Thread Gustavo F. Padovan
---
 plugins/phonesim.c |   10 ++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/plugins/phonesim.c b/plugins/phonesim.c
index d2faf42..7426da6 100644
--- a/plugins/phonesim.c
+++ b/plugins/phonesim.c
@@ -237,6 +237,13 @@ static void cfun_set_on_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
ofono_modem_set_powered(modem, ok);
 }
 
+static void crst_notify(GAtResult *result, gpointer user_data)
+{
+   struct ofono_modem *modem = user_data;
+
+   ofono_modem_reset(modem);
+}
+
 static void phonesim_disconnected(gpointer user_data)
 {
struct ofono_modem *modem = user_data;
@@ -389,6 +396,9 @@ static int phonesim_enable(struct ofono_modem *modem)
g_at_chat_send(data->chat, "AT+CSCS=\"GSM\"", none_prefix,
NULL, NULL, NULL);
 
+   g_at_chat_register(data->chat, "+CRST:",
+   crst_notify, FALSE, modem, NULL);
+
return 0;
 }
 
-- 
1.7.3.1

___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


[RFC 1/3] Simplify ofono_modem_set_powered() logic

2010-11-12 Thread Gustavo F. Padovan
---
 src/modem.c |   47 +--
 1 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index 3776461..3fb6809 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -752,6 +752,7 @@ static GDBusSignalTable modem_signals[] = {
 void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
 {
DBusConnection *conn = ofono_dbus_get_connection();
+   dbus_bool_t dbus_powered = powered;
 
if (modem->timeout > 0) {
g_source_remove(modem->timeout);
@@ -771,32 +772,34 @@ void ofono_modem_set_powered(struct ofono_modem *modem, 
ofono_bool_t powered)
 
modem->powered_pending = powered;
 
-   if (modem->powered != powered) {
-   dbus_bool_t dbus_powered = powered;
-   modem->powered = powered;
+   if (modem->powered == powered)
+   goto out;
 
-   if (modem->driver == NULL) {
-   ofono_error("Calling ofono_modem_set_powered on a"
-   "modem with no driver is not valid, "
-   "please fix the modem driver.");
-   return;
-   }
+   modem->powered = powered;
 
-   ofono_dbus_signal_property_changed(conn, modem->path,
-   OFONO_MODEM_INTERFACE,
-   "Powered", DBUS_TYPE_BOOLEAN,
-   &dbus_powered);
+   if (modem->driver == NULL) {
+   ofono_error("Calling ofono_modem_set_powered on a"
+   "modem with no driver is not valid, "
+   "please fix the modem driver.");
+   return;
+   }
 
-   if (powered) {
-   modem_change_state(modem, MODEM_STATE_PRE_SIM);
+   ofono_dbus_signal_property_changed(conn, modem->path,
+   OFONO_MODEM_INTERFACE,
+   "Powered", DBUS_TYPE_BOOLEAN,
+   &dbus_powered);
 
-   /* Force SIM Ready for devies with no sim atom */
-   if (__ofono_modem_find_atom(modem,
-   OFONO_ATOM_TYPE_SIM) == NULL)
-   sim_state_watch(OFONO_SIM_STATE_READY, modem);
-   } else
-   modem_change_state(modem, MODEM_STATE_POWER_OFF);
-   }
+   if (powered) {
+   modem_change_state(modem, MODEM_STATE_PRE_SIM);
+
+   /* Force SIM Ready for devies with no sim atom */
+   if (__ofono_modem_find_atom(modem,
+   OFONO_ATOM_TYPE_SIM) == NULL)
+   sim_state_watch(OFONO_SIM_STATE_READY, modem);
+   } else
+   modem_change_state(modem, MODEM_STATE_POWER_OFF);
+
+out:
 
if (powering_down && powered == FALSE) {
modems_remaining -= 1;
-- 
1.7.3.1

___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


[RFC 2/3] Add ofono_modem_reset()

2010-11-12 Thread Gustavo F. Padovan
Some modems can screw up everything and then we will need to do a silent
reset of the modem. This patch take the modem back to the OFFLINE state.
---
 include/modem.h |2 +
 src/modem.c |   57 +-
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/include/modem.h b/include/modem.h
index 7b13ee0..a92eb88 100644
--- a/include/modem.h
+++ b/include/modem.h
@@ -46,6 +46,8 @@ int ofono_modem_register(struct ofono_modem *modem);
 ofono_bool_t ofono_modem_is_registered(struct ofono_modem *modem);
 void ofono_modem_remove(struct ofono_modem *modem);
 
+void ofono_modem_reset(struct ofono_modem *modem);
+
 void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered);
 ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem);
 
diff --git a/src/modem.c b/src/modem.c
index 3fb6809..f6d7585 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -70,6 +70,7 @@ struct ofono_modem {
guint   interface_update;
ofono_bool_tpowered;
ofono_bool_tpowered_pending;
+   ofono_bool_treset_pending;
guint   timeout;
ofono_bool_tonline;
GHashTable  *properties;
@@ -784,7 +785,8 @@ void ofono_modem_set_powered(struct ofono_modem *modem, 
ofono_bool_t powered)
return;
}
 
-   ofono_dbus_signal_property_changed(conn, modem->path,
+   if (!modem->reset_pending)
+   ofono_dbus_signal_property_changed(conn, modem->path,
OFONO_MODEM_INTERFACE,
"Powered", DBUS_TYPE_BOOLEAN,
&dbus_powered);
@@ -799,14 +801,34 @@ void ofono_modem_set_powered(struct ofono_modem *modem, 
ofono_bool_t powered)
} else
modem_change_state(modem, MODEM_STATE_POWER_OFF);
 
-out:
+   if (modem->reset_pending && !powering_down) {
+   int err;
+
+   modem->reset_pending = FALSE;
+
+   if (modem->powered) {
+   modem_change_state(modem, MODEM_STATE_OFFLINE);
+   } else {
+   err = set_powered(modem, TRUE);
+   if (err == -EINPROGRESS) {
+   modem->reset_pending = TRUE;
+   return;
+   }
+
+   modem_change_state(modem, MODEM_STATE_PRE_SIM);
+
+   modem_change_state(modem, MODEM_STATE_OFFLINE);
+   }
+   }
 
+out:
if (powering_down && powered == FALSE) {
modems_remaining -= 1;
 
if (modems_remaining == 0)
__ofono_exit();
}
+
 }
 
 ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem)
@@ -1566,6 +1588,37 @@ void ofono_modem_remove(struct ofono_modem *modem)
g_free(modem);
 }
 
+static gboolean __reset_modem(void *data)
+{
+   struct ofono_modem *modem = data;
+   int err;
+
+   err = set_powered(modem, FALSE);
+   if (err == -EINPROGRESS) {
+   modem->reset_pending = TRUE;
+   return FALSE;
+   }
+
+   err = set_powered(modem, TRUE);
+   if (err == -EINPROGRESS) {
+   modem->reset_pending = TRUE;
+   return FALSE;
+   }
+
+   modem_change_state(modem, MODEM_STATE_PRE_SIM);
+
+   modem_change_state(modem, MODEM_STATE_OFFLINE);
+
+   return FALSE;
+}
+
+void ofono_modem_reset(struct ofono_modem *modem)
+{
+   DBG("%p", modem);
+
+   g_idle_add(__reset_modem, modem);
+}
+
 int ofono_modem_driver_register(const struct ofono_modem_driver *d)
 {
DBG("driver: %p, name: %s", d, d->name);
-- 
1.7.3.1

___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


[ANNOUNCE] oFono 0.35

2010-11-12 Thread Denis Kenzior
Hi Everyone,

oFono 0.35 is out.  This release is mostly a development release.  We
also include a bug fix that precluded 2G sims from working correctly due
to FDN/BDN checks.  3G sims were not affected.

The first major change in this release is the included support of WAP
PUSH notifications.  oFono now provides a way for the system to listen
to SMS messages containing WAP PUSH PDUs.  Please see
doc/push-notification-api.txt for more details.

The other major change is the included support for SMS messages
formatted according to the Smart Messaging specification from Nokia.
This allows applications to send and receive vCard and vCalendar objects
over SMS.  Traditionally this is used for e.g. sending business cards
and appointments over SMS.  Please see doc/smart-messaging-api.txt for
more details.

ChangeLog:
- Fix issue with FDN and BDN enabled checks.
- Fix issue with capabilities and Phonet support.
- Fix issue with timeout for ISI network deregistration.
- Add support for Push Notification interface.
- Add support for Smart Messaging interface.
- Remove generic AT command modem plugin.

The signed tarballs for oFono 0.35 can be found on kernel.org [1] [2].

[1] http://www.kernel.org/pub/linux/network/ofono/ofono-0.35.tar.bz2
[2] http://www.kernel.org/pub/linux/network/ofono/ofono-0.35.tar.bz2.sign

Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


RE: [PATCH] TODO: Add vCard export to SM/ME stores

2010-11-12 Thread Marcel Holtmann
Hi Jaakko,

> > To support this feature then first we need to convert the current
> > feature into returning a dict. And then have this feature using a dict
> > as input.
> 
> Is there already a specification/draft of the format of this dict? If not, I 
> would be tempted to use the 27.007 +CPBR/W field names as keys (e.g. index, 
> number, type, text, adnumber, secondtext, sip_uri, etc.)

there is not. So you need to propose one here.

> I'm assuming you would then want to have "aa{ss} Import(void)" to import and 
> array of contact info dicts from all stores, and respectively "void Export(s, 
> aa{ss})" to export one or more contact info dicts to a specified store?

It would be "aa{sv} Import()" since I want a proper dict with sv and not
just ss. Also "Export(aa{sv})" since it is all or nothing. The export
would delete any other entries in the phonebook.

And let me be pretty clear here. Writing a phonebook to the SIM is still
stupid. It is fully pointless in a modern smartphone. The only reason
why we would be merging such a feature upstream is for weird Chinese
type approval and nothing else.

Any UI designer that thinks exposing this is a good idea, should find
himself/herself a new job ;)

Regards

Marcel


___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


[PATCH v5b 3/3] stemodem: Use RTNL to create interfaces

2010-11-12 Thread Sjur Brændeland
From: Sjur Brændeland 

Use rtnl_caif_* functions to create network interfaces.
The conn_info struct is also re-structured in order to
hold the relevant information for network interfaces.
---
 drivers/stemodem/gprs-context.c |   76 ++-
 1 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index 62643ee..8b5cfdc 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -47,6 +47,7 @@
 #include "stemodem.h"
 #include "caif_socket.h"
 #include "if_caif.h"
+#include "caif_rtnl.h"
 
 #define MAX_CAIF_DEVICES 4
 #define MAX_DNS 2
@@ -66,10 +67,18 @@ 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 {
@@ -77,7 +86,6 @@ struct eppsd_response {
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];
@@ -97,8 +105,6 @@ static void start_element_handler(GMarkupParseContext 
*context,
rsp->current = rsp->subnet_mask;
else if (!strcmp(element_name, "mtu"))
rsp->current = rsp->mtu;
-   else if (!strcmp(element_name, "default_gateway"))
-   rsp->current = rsp->default_gateway;
else if (!strcmp(element_name, "dns_server") &&
rsp->dns_server1[0] == '\0')
rsp->current = rsp->dns_server1;
@@ -124,7 +130,7 @@ static void text_handler(GMarkupParseContext *context,
 
if (rsp->current) {
strncpy(rsp->current, text, IP_ADDR_LEN);
-   rsp->current[IP_ADDR_LEN] = 0;
+   rsp->current[IP_ADDR_LEN] = '\0';
}
 }
 
@@ -167,12 +173,38 @@ static struct conn_info *conn_info_create(unsigned int 
channel_id)
return connection;
 }
 
+static void rtnl_callback(int ifindex, char *ifname, void *user_data)
+{
+   struct conn_info *conn = user_data;
+
+   if (ifindex < 0) {
+   conn->created = FALSE;
+   ofono_error("Failed to create caif interface %s",
+   conn->interface);
+   return;
+   }
+
+   strncpy(conn->interface, ifname, sizeof(conn->interface));
+   conn->ifindex = ifindex;
+   conn->created = TRUE;
+}
+
 /*
  * Creates a new IP interface for CAIF.
  */
 static gboolean caif_if_create(struct conn_info *conn)
 {
-   return FALSE;
+   int err;
+
+   err = caif_rtnl_create_interface(IFLA_CAIF_IPV4_CONNID,
+   conn->channel_id, FALSE,
+   rtnl_callback, conn);
+   if (err < 0) {
+   DBG("Failed to create IP interface for CAIF");
+   return FALSE;
+   }
+
+   return TRUE;
 }
 
 /*
@@ -180,6 +212,18 @@ static gboolean caif_if_create(struct conn_info *conn)
  */
 static void caif_if_remove(struct conn_info *conn)
 {
+   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,
@@ -283,7 +327,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
dns[2] = NULL;
 
CALLBACK_WITH_SUCCESS(cb, conn->interface, TRUE, rsp.ip_address,
-   rsp.subnet_mask, rsp.default_gateway,
+   rsp.subnet_mask, NULL,
dns, cbd->data);
return;
 
@@ -380,6 +424,11 @@ static void ste_gprs_activate_primary(struct 
ofono_gprs_context *gc,
}
 
conn = l->data;
+   if (!conn->created) {
+   DBG("CAIF interface not created (rtnl error?)");
+   goto error;
+   }
+
conn->cid = ctx->cid;
 
len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"", ctx->cid);
@@ -460,6 +509,7 @@ static void ste_cgact_read_cb(gboolean ok, GAtResult 
*result,
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc

[PATCH v5b 2/3] stemodem: Create network interfaces statically

2010-11-12 Thread Sjur Brændeland
From: Sjur Brændeland 

Create the network interfaces statically at start up
instead of dynamically for each PDP activation.
---
 drivers/stemodem/gprs-context.c |   98 --
 1 files changed, 52 insertions(+), 46 deletions(-)

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index 05fec3f..62643ee 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -153,8 +154,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,7 +162,6 @@ 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;
@@ -171,7 +170,7 @@ static struct conn_info *conn_info_create(unsigned int 
device,
 /*
  * 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;
 }
@@ -179,9 +178,8 @@ static gboolean caif_if_create(const char *interface, 
unsigned int connid)
 /*
  * 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;
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
@@ -191,7 +189,7 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult 
*result,
ofono_gprs_context_cb_t cb = cbd->cb;
struct ofono_gprs_context *gc = cbd->user;
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-   struct conn_info *conn;
+   struct conn_info *conn = NULL;
GSList *l;
 
if (!ok) {
@@ -214,13 +212,8 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult 
*result,
}
 
conn = l->data;
-
-   if (!caif_if_remove(conn->interface, conn->channel_id)) {
-   DBG("Failed to remove caif interface %s.",
-   conn->interface);
-   }
-
conn->cid = 0;
+   CALLBACK_WITH_SUCCESS(cb, cbd->data);
return;
 
 error:
@@ -248,9 +241,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
conn_compare_by_cid);
 
if (!l) {
-   DBG("Did not find data (device and channel id)"
-   "for connection with cid; %d",
-   gcd->active_context);
+   DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
goto error;
}
 
@@ -291,20 +282,9 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult 
*result, gpointer user_data)
dns[1] = rsp.dns_server2;
dns[2] = NULL;
 
-   sprintf(conn->interface, "caif%u", conn->device);
-
-   if (!caif_if_create(conn->interface, conn->channel_id)) {
-   ofono_error("Failed to create caif interface %s.",
-   conn->interface);
-   CALLBACK_WITH_SUCCESS(cb, NULL, FALSE, rsp.ip_address,
+   CALLBACK_WITH_SUCCESS(cb, conn->interface, TRUE, rsp.ip_address,
rsp.subnet_mask, rsp.default_gateway,
dns, cbd->data);
-   } else {
-   CALLBACK_WITH_SUCCESS(cb, conn->interface,
-   FALSE, rsp.ip_address, rsp.subnet_mask,
-   rsp.default_gateway, dns, cbd->data);
-   }
-
return;
 
 error:
@@ -316,6 +296,7 @@ error:
if (conn)
conn->cid = 0;
 
+   gcd->active_context = 0;
CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
 }
 
@@ -327,38 +308,43 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult 
*result, gpointer user_data)
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
struct cb_data *ncbd = NULL;
char buf[128];
-   struct conn_info *conn;
+   struct conn_info *conn = NULL;
GSList *l;
 
+   l = g_slist_find_custom(g_caif_devices,
+   GUINT_TO_POINTER(gcd->active_context),
+   conn_compare_by_cid);
+
+   if (!l) {
+   DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
+   goto error;
+   }
+
+   conn = l->data;
+
if (!ok) {
struct ofono_error error;
 
+   conn->cid = 0;
gcd->active_context = 

[PATCH v5b 1/3] stemodem: Fix for error handling, memleak and changed some defines

2010-11-12 Thread Sjur Brændeland
From: Sjur Brændeland 

* renamed MAX_LEN to IP_ADDR_LEN
* removed memory leak from unneeded strdup when parsing xml response.
* better handling of AT error responses
* reduced number of caif interfaces to 4
---
 drivers/stemodem/gprs-context.c |   54 ++
 1 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index 9f59579..05fec3f 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -47,9 +47,9 @@
 #include "caif_socket.h"
 #include "if_caif.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)
@@ -73,13 +73,13 @@ struct conn_info {
 
 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 +122,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;
}
 }
 
@@ -191,12 +191,16 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult 
*result,
ofono_gprs_context_cb_t cb = cbd->cb;
struct ofono_gprs_context *gc = cbd->user;
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-   struct ofono_error error;
struct conn_info *conn;
GSList *l;
 
-   if (!ok)
-   goto error;
+   if (!ok) {
+   struct ofono_error error;
+
+   decode_at_error(&error, g_at_result_final_response(result));
+   cb(&error, cbd->data);
+   return;
+   }
 
l = g_slist_find_custom(g_caif_devices,
GUINT_TO_POINTER(gcd->active_context),
@@ -217,9 +221,6 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult 
*result,
}
 
conn->cid = 0;
-
-   decode_at_error(&error, g_at_result_final_response(result));
-   cb(&error, cbd->data);
return;
 
 error:
@@ -237,7 +238,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
GSList *l;
int i;
gsize length;
-   char *res_string;
+   const char *res_string;
const char *dns[MAX_DNS + 1];
struct eppsd_response rsp;
GMarkupParseContext *context = NULL;
@@ -255,8 +256,15 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult 
*result, gpointer user_data)
 
conn = l->data;
 
-   if (!ok)
-   goto error;
+   if (!ok) {
+   struct ofono_error error;
+
+   conn->cid = 0;
+   gcd->active_context = 0;
+   decode_at_error(&error, g_at_result_final_response(result));
+   cb(&error, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
+   return;
+   }
 
rsp.current = NULL;
context = g_markup_parse_context_new(&parser, 0, &rsp, NULL);
@@ -266,7 +274,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
 
for (i = 0; i < g_at_result_num_response_lines(result); i++) {
g_at_result_iter_next(&iter, NULL);
-   res_string = strdup(g_at_result_iter_raw_line(&iter));
+   res_string = g_at_result_iter_raw_line(&iter);
length = strlen(res_string);
 
if (!g_markup_parse_context_parse(context, res_string,
@@ -326,7 +334,6 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
struct ofono_error error;
 
gcd->active_context = 0;
-
decode_at_error(&error, g_at_result_final_response(result));
cb(&error, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
return;
@@ -389,7 +396,7 @@ static void ste_gprs_activate_primary(struct 
ofono_gprs_context *gc,
 * Set username and password, this should be done after CGDCONT
 * or an error can occur.  We don't bother with error checking
 * here
-* */
+*/
snprintf(buf, sizeof(buf), "AT*EIAAUW=%d,1,\"%s\",\"%s\"",
ctx->cid,

[PATCH v5b 0/3] stemodem: Add RTNL handling to gprs-context.c

2010-11-12 Thread Sjur Brændeland
From: Sjur Brændeland 

This patch-set applies on top of:
"[PATCH v5] stemodem: Add RTNL functionality managing CAIF Network Interfaces."

I have broken this patch-set into three separate patches as requested.
1) Bug-fixes and renaming
2) Restructuring for creating interfaces statically
3) Introducing calls to caif_rtnl_* to actually use the network interfaces.

Regards,
Sjur

Sjur Brændeland (3):
  stemodem: Fix for error handling, memleak and changed some defines
  stemodem: Create network interfaces statically
  stemodem: Use RTNL to create interfaces

 drivers/stemodem/gprs-context.c |  222 ++-
 1 files changed, 147 insertions(+), 75 deletions(-)

___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


Re: Can I modify CallingLineRestriction(the Property of CallSetting) value to "disabled" or "enabled"

2010-11-12 Thread Denis Kenzior
Hi Pekka,

On 11/12/2010 08:19 AM, Pekka Pessi wrote:
> Hi,
> 
> 2010/11/11 Gu, Yang :
>>> What happens when you make a call? Does call get rejected or do you
>>> get some CSSU indications?
>>
>> Thank you for the comments!
>> After setting HideCallerId to "enabled", and making an outgoing call, the 
>> call arrived at another phone as normal, and I didn't see any CSSU 
>> indication. The call settings are listed below, in which the 
>> CallingLineRestriction is always disabled. I think this means the network 
>> disables this feature. I also confirmed with our operator (CMCC), and they 
>> said they don't support CLIR.
> 
> They really do not support it. Thanks for the logs.
> 
> There is a Finnish operator (Aino) which does not have CLIR
> provisioned by default, they will reject the calls made with temporary
> CLIR invocation.
> 
> It seems to me that the call-settings API stores the user preference
> to the modem (how that is done is up to the modem). This can lead to
> subtle privacy bugs if user has multiple SIM cards. I wonder if oFono
> should store the CLIR settings in IMSI-specific storage. oFono should
> somehow specify the interaction with the CLIR setting stored in the
> modem, too.
> 

Are you sure?  I actually assume that the network simply replies OK to
the CLIR invocation but doesn't actually honor it.  If this is the case
then no amount of API behavioral changes is going to help here.

Also remember that this is Huawei we're dealing with here.  Have you
tried other vendors?

Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


RE: [PATCH v4 2/2] stemodem: Update gprs-context to use rtnl to create/remove interfaces.

2010-11-12 Thread Sjur BRENDELAND
Hi Marcel.
 
> Can you please not try to squeeze such change in the same patch. Just
> have a separate one that does this change.
...
> The more I looked through the rest of this patch, the more I thought
> you need to split this patch into a cleanup and caif_rtnl usage patch.

Yes, I have broken this into three patches.
It's a bit hard to find the exactly what to put where, and
tear apart what is already squashed. But have a look and
see what you think.

> 
> So I realized now that you are using ifindex as input for
> caif_rtnl_delete_interface, now I am questioning why there is a result
> parameter in the callback for caif_rtnl_create_interface.

OK, agree - done.

> Why are we having this in the first place. CAIF network interfaces are
> point-to-point, right? In that case you should just leave this NULL.

Yes - agree. PtP link should not report GW.

> Please use '\0' instead of just 0. You might need to fix this at other
> places as well.

OK, done at least here..

> > +static void rtnl_callback(int result, gpointer user_data,
> > +   char *ifname, int ifindex)
> > +{
> > +   struct conn_info *conn = user_data;
> > +
> > +   strncpy(conn->interface, ifname, sizeof(conn->interface));
> > +   conn->ifindex = ifindex;
> > +
> > +   if (result == 0)
> > +   conn->created = TRUE;
> > +   else {
> > +   conn->created = FALSE;
> > +   DBG("Could not create CAIF Interface");
> > +   }
> > +}
> 
> So here is the think. This makes no sense. Either you get an error and
> ifname and index are not valid or you succeed.
> 
> So this should be something like
> 
>   if (ifname < 0) {
>   connman_error(...)
>   return;
>   }
OK, I've done something similar.


Regards,
Sjur




___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


Re: Can I modify CallingLineRestriction(the Property of CallSetting) value to "disabled" or "enabled"

2010-11-12 Thread Pekka Pessi
Hi,

2010/11/11 Gu, Yang :
>>What happens when you make a call? Does call get rejected or do you
>>get some CSSU indications?
>
> Thank you for the comments!
> After setting HideCallerId to "enabled", and making an outgoing call, the 
> call arrived at another phone as normal, and I didn't see any CSSU 
> indication. The call settings are listed below, in which the 
> CallingLineRestriction is always disabled. I think this means the network 
> disables this feature. I also confirmed with our operator (CMCC), and they 
> said they don't support CLIR.

They really do not support it. Thanks for the logs.

There is a Finnish operator (Aino) which does not have CLIR
provisioned by default, they will reject the calls made with temporary
CLIR invocation.

It seems to me that the call-settings API stores the user preference
to the modem (how that is done is up to the modem). This can lead to
subtle privacy bugs if user has multiple SIM cards. I wonder if oFono
should store the CLIR settings in IMSI-specific storage. oFono should
somehow specify the interaction with the CLIR setting stored in the
modem, too.

-- 
Pekka.Pessi mail at nokia.com
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


[PATCH v5] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

2010-11-12 Thread Sjur Brændeland
From: Sjur Brændeland 

---
 Makefile.am  |2 +
 drivers/stemodem/caif_rtnl.c |  340 ++
 drivers/stemodem/caif_rtnl.h |   29 
 3 files changed, 371 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..4ce2401
--- /dev/null
+++ b/drivers/stemodem/caif_rtnl.c
@@ -0,0 +1,340 @@
+/*
+ *
+ *  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)))
+
+#define RTNL_MSG_SIZE 4096
+
+struct rtnl_msg {
+   struct nlmsghdr n;
+   struct ifinfomsg i;
+   char data[RTNL_MSG_SIZE];
+};
+
+struct iplink_req {
+   guint32 rtnlmsg_seqnr;
+   gpointer user_data;
+   caif_rtnl_create_cb_t callback;
+};
+
+static GSList *pending_requests;
+static guint32 rtnl_seqnr;
+static guint rtnl_watch;
+static GIOChannel *rtnl_channel;
+
+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->rtnlmsg_seqnr == seq)
+   return req;
+   }
+
+   return NULL;
+}
+
+static void parse_newlink_param(struct ifinfomsg *msg, int size,
+   int *ifindex, char *ifname)
+{
+   struct rtattr *attr;
+
+   for (attr = IFLA_RTA(msg); RTA_OK(attr, size);
+   attr = RTA_NEXT(attr, size)) {
+
+   if (attr->rta_type == IFLA_IFNAME &&
+   ifname != NULL) {
+
+   strncpy(ifname, RTA_DATA(attr), IF_NAMESIZE);
+   ifname[IF_NAMESIZE-1] = '\0';
+   break;
+   }
+   }
+
+   *ifindex = msg->ifi_index;
+}
+
+static void parse_rtnl_message(const void *buf, size_t len)
+{
+   struct ifinfomsg *msg;
+   struct iplink_req *req = NULL;
+   char ifname[IF_NAMESIZE];
+   int ifindex;
+
+   while (len > 0) {
+   const struct nlmsghdr *hdr = buf;
+
+   if (!NLMSG_OK(hdr, len))
+   break;
+
+   switch (hdr->nlmsg_type) {
+   case RTM_NEWLINK:
+   req = g_slist_nth_data(pending_requests, 0);
+   if (req == NULL)
+   break;
+
+   msg = (struct ifinfomsg *) NLMSG_DATA(hdr);
+   parse_newlink_param(msg, IFA_PAYLOAD(hdr),
+   &ifindex, ifname);
+
+   if (req->callback)
+   req->callback(ifindex, ifname, req->user_data);
+   break;
+
+   case NLMSG_ERROR:
+   req = find_request(hdr->nlmsg_seq);
+   if (req == NULL)
+   break;
+
+   DBG("nlmsg error req");
+   if (req->callback)
+   req->callback(-1, ifname, req->user_data);
+   break;
+   default:
+   break;
+   }
+
+   len -= hdr->nlmsg_len;
+   buf += hdr-

RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

2010-11-12 Thread Sjur BRENDELAND
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 

RE: [PATCH] TODO: Add vCard export to SM/ME stores

2010-11-12 Thread Kiviluoto, Jaakko J
> > diff --git a/TODO b/TODO
> > index bf2305b..9dcb43f 100644
> > --- a/TODO
> > +++ b/TODO
> > @@ -496,3 +496,14 @@ Miscellaneous
> >
> >Priority: Low
> >Complexity: C4
> > +
> > +- Enable exporting contact information from vCard data to SM and ME
> stores.
> > +  Need to implement a robust vCard parser that can extract at least
> the
> > +  fields supported by the existing phonebook module.  Functionality
> should
> > +  be analoguous to existing import functionality.
> 
> and you will not be able to write a robust vCard parser that we can
> safely run as root or with any kind of ofonod privileges. It is just
> way
> too risky from a security point of view.

Got it. (I do have a parser implementation 95% ready just in case you're 
willing to try this another time.)

> To support this feature then first we need to convert the current
> feature into returning a dict. And then have this feature using a dict
> as input.

Is there already a specification/draft of the format of this dict? If not, I 
would be tempted to use the 27.007 +CPBR/W field names as keys (e.g. index, 
number, type, text, adnumber, secondtext, sip_uri, etc.)

I'm assuming you would then want to have "aa{ss} Import(void)" to import and 
array of contact info dicts from all stores, and respectively "void Export(s, 
aa{ss})" to export one or more contact info dicts to a specified store?

Jaakko

-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


Re: [PATCH] TODO: Add vCard export to SM/ME stores

2010-11-12 Thread Marcel Holtmann
Hi Jaakko,

>  TODO |   11 +++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/TODO b/TODO
> index bf2305b..9dcb43f 100644
> --- a/TODO
> +++ b/TODO
> @@ -496,3 +496,14 @@ Miscellaneous
>  
>Priority: Low
>Complexity: C4
> +
> +- Enable exporting contact information from vCard data to SM and ME stores.
> +  Need to implement a robust vCard parser that can extract at least the
> +  fields supported by the existing phonebook module.  Functionality should
> +  be analoguous to existing import functionality.

and you will not be able to write a robust vCard parser that we can
safely run as root or with any kind of ofonod privileges. It is just way
too risky from a security point of view.

To support this feature then first we need to convert the current
feature into returning a dict. And then have this feature using a dict
as input.

Regards

Marcel


___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono