[PATCH 0/3] Unify SPN reading logic in src/gprs.c

[PATCH 0/3] Unify SPN reading logic in src/gprs.c

[Oleg Zhurakivskyy]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

Hello,

Please find the changes in order not to duplicate SPN reading logic in src/grps.c.

Regards,
Oleg

Oleg Zhurakivskyy (3):
  gprs: Minor whitespace and style fixes
  network: Add SPN watch capability
  gprs: Use netreg SPN watch API

 src/gprs.c    |   91 +++++++++++++++++++++++++++++-------------------------
 src/network.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 src/ofono.h   |    7 ++++
 3 files changed, 146 insertions(+), 47 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] gprs: Minor whitespace and style fixes

[Oleg Zhurakivskyy]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

---
 src/gprs.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 90d3a6d..4e46743 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -57,9 +57,6 @@
 #define MAX_CONTEXTS 256
 #define SUSPEND_TIMEOUT 8
 
-static GSList *g_drivers = NULL;
-static GSList *g_context_drivers = NULL;
-
 /* 27.007 Section 7.29 */
 enum packet_bearer {
 	PACKET_BEARER_NONE =		0,
@@ -152,6 +149,9 @@ struct pri_context {
 static void gprs_netreg_update(struct ofono_gprs *gprs);
 static void gprs_deactivate_next(struct ofono_gprs *gprs);
 
+static GSList *g_drivers = NULL;
+static GSList *g_context_drivers = NULL;
+
 const char *packet_bearer_to_string(int bearer)
 {
 	switch (bearer) {
@@ -1285,7 +1285,7 @@ static DBusMessage *pri_set_property(DBusConnection *conn,
 static GDBusMethodTable context_methods[] = {
 	{ "GetProperties",	"",	"a{sv}",	pri_get_properties },
 	{ "SetProperty",	"sv",	"",		pri_set_property,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
 
@@ -1404,11 +1404,11 @@ static void update_suspended_property(struct ofono_gprs *gprs,
 
 static gboolean suspend_timeout(gpointer data)
 {
-       struct ofono_gprs *gprs = data;
+	struct ofono_gprs *gprs = data;
 
-       gprs->suspend_timeout = 0;
-       update_suspended_property(gprs, TRUE);
-       return FALSE;
+	gprs->suspend_timeout = 0;
+	update_suspended_property(gprs, TRUE);
+	return FALSE;
 }
 
 void ofono_gprs_suspend_notify(struct ofono_gprs *gprs, int cause)
@@ -2253,7 +2253,8 @@ void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
 	}
 }
 
-int ofono_gprs_context_driver_register(const struct ofono_gprs_context_driver *d)
+int ofono_gprs_context_driver_register(
+				const struct ofono_gprs_context_driver *d)
 {
 	DBG("driver: %p, name: %s", d, d->name);
 
@@ -2265,7 +2266,8 @@ int ofono_gprs_context_driver_register(const struct ofono_gprs_context_driver *d
 	return 0;
 }
 
-void ofono_gprs_context_driver_unregister(const struct ofono_gprs_context_driver *d)
+void ofono_gprs_context_driver_unregister(
+				const struct ofono_gprs_context_driver *d)
 {
 	DBG("driver: %p, name: %s", d, d->name);
 
@@ -2346,7 +2348,7 @@ struct ofono_modem *ofono_gprs_context_get_modem(struct ofono_gprs_context *gc)
 }
 
 void ofono_gprs_context_set_type(struct ofono_gprs_context *gc,
-                                        enum ofono_gprs_context_type type)
+					enum ofono_gprs_context_type type)
 {
 	DBG("type %d", type);
 
-- 
1.7.5.4

Re: [PATCH 1/3] gprs: Minor whitespace and style fixes

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]

Hi Oleg,

On 12/28/2011 07:18 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/gprs.c |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

[PATCH 2/3] network: Add SPN watch capability

[Oleg Zhurakivskyy]

[-- Attachment #1: Type: text/plain, Size: 4893 bytes --]

---
 src/network.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 src/ofono.h   |    7 ++++
 2 files changed, 97 insertions(+), 5 deletions(-)

diff --git a/src/network.c b/src/network.c
index 92256a8..9a685d4 100644
--- a/src/network.c
+++ b/src/network.c
@@ -84,6 +84,7 @@ struct ofono_netreg {
 	struct ofono_atom *atom;
 	unsigned int hfp_watch;
 	char *spn;
+	struct ofono_watchlist *spn_watches;
 };
 
 struct network_operator_data {
@@ -1638,6 +1639,69 @@ static void sim_spdi_read_cb(int ok, int length, int record,
 	netreg_emit_operator_display_name(netreg);
 }
 
+gboolean __ofono_netreg_add_spn_watch(struct ofono_netreg *netreg,
+					unsigned int *id, GFunc cb,
+					void *data, ofono_destroy_func destroy)
+{
+	struct ofono_watchlist_item *item;
+	unsigned int watch_id;
+
+	DBG("%p", netreg);
+
+	if (netreg == NULL)
+		return 0;
+
+	item = g_new0(struct ofono_watchlist_item, 1);
+
+	item->notify = cb;
+	item->destroy = destroy;
+	item->notify_data = data;
+
+	watch_id = __ofono_watchlist_add_item(netreg->spn_watches, item);
+	if (watch_id == 0)
+		return FALSE;
+
+	*id = watch_id;
+
+	if (item->notify != NULL && netreg->spn != NULL)
+		cb(netreg->spn, item->notify_data);
+
+	return TRUE;
+}
+
+gboolean __ofono_netreg_remove_spn_watch(struct ofono_netreg *netreg,
+						unsigned int *id)
+{
+	gboolean ret;
+
+	DBG("%p", netreg);
+
+	if (netreg == NULL)
+		return FALSE;
+
+	ret = __ofono_watchlist_remove_item(netreg->spn_watches, *id);
+	if (ret == TRUE)
+		*id = 0;
+
+	return ret;
+}
+
+static void spn_watch_cb(gpointer data, gpointer user_data)
+{
+	struct ofono_watchlist_item *item = data;
+
+	if (item->notify)
+		((GFunc) item->notify)(user_data, item->notify_data);
+}
+
+static inline void spn_watches_notify(struct ofono_netreg *netreg)
+{
+	if (netreg->spn_watches->items == NULL)
+		return;
+
+	g_slist_foreach(netreg->spn_watches->items, spn_watch_cb, netreg->spn);
+}
+
 static void sim_spn_display_condition_parse(struct ofono_netreg *netreg,
 						guint8 dcbyte)
 {
@@ -1691,10 +1755,16 @@ static void sim_cphs_spn_short_read_cb(int ok, int length, int record,
 
 	netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
 
-	if (!ok)
+	if (!ok) {
+		spn_watches_notify(netreg);
 		return;
+	}
+
+	sim_spn_parse(data, length, &netreg->spn);
 
-	if (!sim_spn_parse(data, length, &netreg->spn))
+	spn_watches_notify(netreg);
+
+	if (netreg->spn == NULL)
 		return;
 
 	if (netreg->current_operator)
@@ -1714,15 +1784,21 @@ static void sim_cphs_spn_read_cb(int ok, int length, int record,
 					SIM_EF_CPHS_SPN_SHORT_FILEID,
 					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 					sim_cphs_spn_short_read_cb, netreg);
-		else
+		else {
 			netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
+			spn_watches_notify(netreg);
+		}
 
 		return;
 	}
 
 	netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
 
-	if (!sim_spn_parse(data, length, &netreg->spn))
+	sim_spn_parse(data, length, &netreg->spn);
+
+	spn_watches_notify(netreg);
+
+	if (netreg->spn == NULL)
 		return;
 
 	if (netreg->current_operator)
@@ -1746,7 +1822,11 @@ static void sim_spn_read_cb(int ok, int length, int record,
 
 	netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
 
-	if (!sim_spn_parse(data + 1, length - 1, &netreg->spn))
+	sim_spn_parse(data + 1, length - 1, &netreg->spn);
+
+	spn_watches_notify(netreg);
+
+	if (netreg->spn == NULL)
 		return;
 
 	sim_spn_display_condition_parse(netreg, data[0]);
@@ -1888,6 +1968,9 @@ static void netreg_unregister(struct ofono_atom *atom)
 
 	__ofono_modem_remove_atom_watch(modem, netreg->hfp_watch);
 
+	__ofono_watchlist_free(netreg->spn_watches);
+	netreg->spn_watches = NULL;
+
 	__ofono_watchlist_free(netreg->status_watches);
 	netreg->status_watches = NULL;
 
@@ -2171,6 +2254,8 @@ void ofono_netreg_register(struct ofono_netreg *netreg)
 		return;
 	}
 
+	netreg->spn_watches = __ofono_watchlist_new(g_free);
+
 	netreg->status_watches = __ofono_watchlist_new(g_free);
 
 	ofono_modem_add_interface(modem, OFONO_NETWORK_REGISTRATION_INTERFACE);
diff --git a/src/ofono.h b/src/ofono.h
index 0e3009e..5427f0c 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -423,6 +423,13 @@ unsigned int __ofono_netreg_add_status_watch(struct ofono_netreg *netreg,
 gboolean __ofono_netreg_remove_status_watch(struct ofono_netreg *netreg,
 						unsigned int id);
 
+gboolean __ofono_netreg_add_spn_watch(struct ofono_netreg *netreg,
+					unsigned int *id, GFunc cb,
+					void *data, ofono_destroy_func destroy);
+
+gboolean __ofono_netreg_remove_spn_watch(struct ofono_netreg *netreg,
+						unsigned int *id);
+
 void __ofono_netreg_set_base_station_name(struct ofono_netreg *netreg,
 						const char *name);
 
-- 
1.7.5.4

[PATCH 3/3] gprs: Use netreg SPN watch API

[Oleg Zhurakivskyy]

[-- Attachment #1: Type: text/plain, Size: 4595 bytes --]

---
 src/gprs.c |   67 ++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 4e46743..356479c 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -46,8 +46,9 @@
 #include "simutil.h"
 #include "util.h"
 
-#define GPRS_FLAG_ATTACHING 0x1
-#define GPRS_FLAG_RECHECK 0x2
+#define GPRS_FLAG_ATTACHING	0x1
+#define GPRS_FLAG_RECHECK	0x2
+#define GPRS_FLAG_READ_SPN	0x4
 
 #define SETTINGS_STORE "gprs"
 #define SETTINGS_GROUP "Settings"
@@ -87,6 +88,7 @@ struct ofono_gprs {
 	struct ofono_netreg *netreg;
 	unsigned int netreg_watch;
 	unsigned int status_watch;
+	unsigned int spn_watch;
 	GKeyFile *settings;
 	char *imsi;
 	DBusMessage *pending;
@@ -148,6 +150,7 @@ struct pri_context {
 
 static void gprs_netreg_update(struct ofono_gprs *gprs);
 static void gprs_deactivate_next(struct ofono_gprs *gprs);
+static void spn_read_cb(gpointer spn, gpointer user_data);
 
 static GSList *g_drivers = NULL;
 static GSList *g_context_drivers = NULL;
@@ -1528,11 +1531,15 @@ static void gprs_netreg_removed(struct ofono_gprs *gprs)
 {
 	gprs->netreg = NULL;
 
-	gprs->flags &= ~(GPRS_FLAG_RECHECK | GPRS_FLAG_ATTACHING);
+	gprs->flags &= ~(GPRS_FLAG_RECHECK | GPRS_FLAG_ATTACHING |
+						GPRS_FLAG_READ_SPN);
 	gprs->status_watch = 0;
 	gprs->netreg_status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
 	gprs->driver_attached = FALSE;
 
+	if (gprs->spn_watch)
+		__ofono_netreg_remove_spn_watch(gprs->netreg, &gprs->spn_watch);
+
 	gprs_attached_update(gprs);
 }
 
@@ -2625,6 +2632,12 @@ static void netreg_watch(struct ofono_atom *atom,
 					netreg_status_changed, gprs, NULL);
 
 	gprs_netreg_update(gprs);
+
+	if (gprs->flags & GPRS_FLAG_READ_SPN) {
+		__ofono_netreg_add_spn_watch(gprs->netreg, &gprs->spn_watch,
+						spn_read_cb, gprs, NULL);
+		gprs->flags &= ~GPRS_FLAG_READ_SPN;
+	}
 }
 
 static gboolean load_context(struct ofono_gprs *gprs, const char *group)
@@ -2948,34 +2961,29 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 	ofono_modem_add_interface(modem,
 				OFONO_CONNECTION_MANAGER_INTERFACE);
 
-	gprs->netreg_watch = __ofono_modem_add_atom_watch(modem,
-					OFONO_ATOM_TYPE_NETREG,
-					netreg_watch, gprs, NULL);
-
 	__ofono_atom_register(gprs->atom, gprs_unregister);
 }
 
-static void sim_spn_read_cb(int ok, int length, int record,
-				const unsigned char *data,
-				int record_length, void *userdata)
+static void spn_read_cb(gpointer spn, gpointer user_data)
 {
-	struct ofono_gprs *gprs	= userdata;
-	char *spn = NULL;
+	struct ofono_gprs *gprs	= user_data;
 	struct ofono_atom *sim_atom;
-	struct ofono_sim *sim = NULL;
+	struct ofono_sim *sim;
 
-	if (ok)
-		spn = sim_string_to_utf8(data + 1, length - 1);
+	if (gprs->spn_watch == 0)
+		return;
+
+	__ofono_netreg_remove_spn_watch(gprs->netreg, &gprs->spn_watch);
 
 	sim_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(gprs->atom),
 						OFONO_ATOM_TYPE_SIM);
-	if (sim_atom) {
+	if (sim_atom)
 		sim = __ofono_atom_get_data(sim_atom);
+
+	if (sim != NULL && spn != NULL)
 		provision_contexts(gprs, ofono_sim_get_mcc(sim),
 					ofono_sim_get_mnc(sim), spn);
-	}
 
-	g_free(spn);
 	ofono_gprs_finish_register(gprs);
 }
 
@@ -2985,27 +2993,24 @@ void ofono_gprs_register(struct ofono_gprs *gprs)
 	struct ofono_atom *sim_atom;
 	struct ofono_sim *sim = NULL;
 
-	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
+	sim_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(gprs->atom),
+						OFONO_ATOM_TYPE_SIM);
 
 	if (sim_atom) {
-		const char *imsi;
 		sim = __ofono_atom_get_data(sim_atom);
 
-		imsi = ofono_sim_get_imsi(sim);
-		gprs_load_settings(gprs, imsi);
+		gprs_load_settings(gprs, ofono_sim_get_imsi(sim));
 	}
 
-	if (gprs->contexts == NULL && sim != NULL) {
-		/* Get Service Provider Name from SIM for provisioning */
-		gprs->sim_context = ofono_sim_context_create(sim);
+	if (gprs->contexts == NULL && sim != NULL)
+		gprs->flags |= GPRS_FLAG_READ_SPN;
 
-		if (ofono_sim_read(gprs->sim_context, SIM_EFSPN_FILEID,
-				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-					sim_spn_read_cb, gprs) >= 0)
-			return;
-	}
+	gprs->netreg_watch = __ofono_modem_add_atom_watch(modem,
+					OFONO_ATOM_TYPE_NETREG,
+					netreg_watch, gprs, NULL);
 
-	ofono_gprs_finish_register(gprs);
+	if (gprs->spn_watch == 0)
+		ofono_gprs_finish_register(gprs);
 }
 
 void ofono_gprs_remove(struct ofono_gprs *gprs)
-- 
1.7.5.4

Re: [PATCH 3/3] gprs: Use netreg SPN watch API

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

Hi Oleg,

On 12/28/2011 07:18 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/gprs.c |   67 ++++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 36 insertions(+), 31 deletions(-)
> 

So I like this approach, the only problem is that netreg atom is a
post_online atom and gprs is post_sim atom.  This means that if no
contexts are provisioned we must wait for the netreg atom to be
registered.  This leads to a slight asymmetry in the API.

Perhaps we should move the SPN logic to the sim atom and make netreg and
gprs atom simple consumers?

Regards,
-Denis

Re: [PATCH 3/3] gprs: Use netreg SPN watch API

[Oleg Zhurakivskyy]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

Hello Denis,

On 12/29/2011 12:14 AM, Denis Kenzior wrote:
> So I like this approach, the only problem is that netreg atom is a
> post_online atom and gprs is post_sim atom.  This means that if no
> contexts are provisioned we must wait for the netreg atom to be
> registered.  This leads to a slight asymmetry in the API.
>
> Perhaps we should move the SPN logic to the sim atom and make netreg and
> gprs atom simple consumers?

Thanks for the review! Yes, moving the SPN logic into the sim atom will indeed 
simplify the logic, I like the idea.

I will prepare another patchset.

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