[RFC 0/1] GPRS Provisioning Plugin

[RFC 0/1] GPRS Provisioning Plugin

[Oleg Zhurakivskyy]

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

Hello,

Please find the draft of the GPRS provisioning plugin ("Internet Access Provider database" TODO item).

The plugin reads mobile provider settings database entries (PROVIDER_DATABASE) and returns GRPS context settings to oFono provisioning module.

If there are multiple matches found in the database, then some user intervention might be required or database format should be extended, please comment.

Regards,
Oleg

Oleg Zhurakivskyy (1):
  GPRS Provisioning Plugin.

 plugins/gprs-provision.c |  290 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 290 insertions(+), 0 deletions(-)
 create mode 100644 plugins/gprs-provision.c

---------------------------------------------------------------------
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.

[RFC 1/1] GPRS Provisioning Plugin.

[Oleg Zhurakivskyy]

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

---
 plugins/gprs-provision.c |  290 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 290 insertions(+), 0 deletions(-)
 create mode 100644 plugins/gprs-provision.c

diff --git a/plugins/gprs-provision.c b/plugins/gprs-provision.c
new file mode 100644
index 0000000..015ea7e
--- /dev/null
+++ b/plugins/gprs-provision.c
@@ -0,0 +1,290 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
+ *
+ *  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 <config.h>
+#endif
+
+#include <sys/stat.h>
+#include <sys/mman.h>
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <glib.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/types.h>
+#include <ofono/log.h>
+#include <ofono/plugin.h>
+#include <ofono/modem.h>
+#include <ofono/gprs-provision.h>
+
+#define PROVISION_SETTINGS_MAX 10
+
+struct parser_data {
+	struct ofono_gprs_provision_data *settings[PROVISION_SETTINGS_MAX];
+	struct ofono_gprs_provision_data *current;
+	int count;
+	const char *match_mcc;
+	const char *match_mnc;
+	const char *name;
+	const char *current_element;
+	gboolean match_found;
+};
+
+static void parse_element_start(GMarkupParseContext *context,
+				const gchar *element_name,
+				const gchar **attribute_names,
+				const gchar **attribute_values,
+				gpointer user_data, GError **error)
+{
+	struct parser_data *data = user_data;
+
+	if (g_str_equal(element_name, "name") == TRUE) {
+		data->current_element = element_name;
+		return;
+	}
+
+	if (g_str_equal(element_name, "network-id") == TRUE) {
+		const char *mcc = NULL, *mnc = NULL;
+		int i;
+
+		for (i = 0; attribute_names[i]; i++) {
+			if (g_str_equal(attribute_names[i], "mcc") == TRUE)
+				mcc = attribute_values[i];
+			if (g_str_equal(attribute_names[i], "mnc") == TRUE)
+				mnc = attribute_values[i];
+		}
+
+		if (g_strcmp0(mcc, data->match_mcc) == 0 &&
+			g_strcmp0(mnc, data->match_mnc) == 0)
+			data->match_found = TRUE;
+	}
+
+	if (data->match_found == FALSE)
+		return;
+
+	data->current_element = element_name;
+
+	if (g_str_equal(element_name, "apn") == TRUE) {
+		const char *apn = NULL;
+		int i;
+
+		for (i = 0; attribute_names[i]; i++) {
+			if (g_str_equal(attribute_names[i], "value") == TRUE)
+				apn = attribute_values[i];
+		}
+
+		if (data->count >= PROVISION_SETTINGS_MAX) {
+			data->match_found = FALSE;
+			return;
+		}
+
+		data->current = g_try_new0(struct ofono_gprs_provision_data, 1);
+		if (data->current == NULL)
+			return;
+
+		data->settings[data->count++] = data->current;
+
+		if (data->current->apn == NULL)
+			data->current->apn = g_strdup(apn);
+
+		if (data->current->name == NULL)
+			data->current->name = g_strdup(data->name);
+	}
+}
+
+static void parse_element_end(GMarkupParseContext *context,
+				const gchar *element_name,
+				gpointer user_data, GError **error)
+{
+	struct parser_data *data = user_data;
+
+	if (g_str_equal(element_name, "gsm") == TRUE ||
+		g_str_equal(element_name, "cdma") == TRUE) {
+		data->match_found = FALSE;
+		g_free((gpointer)data->name);
+		data->name = NULL;
+	}
+}
+
+static void parse_text(GMarkupParseContext *context,
+			const gchar *text, gsize text_len,
+			gpointer user_data, GError **error)
+{
+	struct parser_data *data = user_data;
+
+	if (g_strcmp0(data->current_element, "name") == 0) {
+		data->name = g_strndup(text, text_len);
+		return;
+	}
+
+	if (data->match_found == FALSE ||
+		(data->current && data->current->apn == NULL))
+		return;
+
+	if (g_strcmp0(data->current_element, "username") == 0)
+		data->current->username = g_strndup(text, text_len);
+	else if (g_strcmp0(data->current_element, "password") == 0)
+		data->current->password = g_strndup(text, text_len);
+}
+
+static void parser_error(GMarkupParseContext *context,
+				GError *error, gpointer user_data)
+{
+	ofono_error("Error parsing %s: %s", PROVIDER_DATABASE, error->message);
+}
+
+static const GMarkupParser parser = {
+	parse_element_start,
+	parse_element_end,
+	parse_text,
+	NULL,
+	parser_error,
+};
+
+static void parse_database(const char *data, ssize_t size,
+	struct parser_data *parser_data)
+{
+	GMarkupParseContext *context;
+	gboolean result;
+
+	parser_data->match_found = FALSE;
+
+	context = g_markup_parse_context_new(&parser,
+						G_MARKUP_TREAT_CDATA_AS_TEXT,
+						parser_data, NULL);
+
+	result = g_markup_parse_context_parse(context, data, size, NULL);
+	if (result == TRUE)
+		result = g_markup_parse_context_end_parse(context, NULL);
+
+	g_markup_parse_context_free(context);
+}
+
+static int lookup_apn(const char *mcc, const char *mnc, const char *spn,
+			struct parser_data *data)
+{
+	struct stat st;
+	char *map;
+	int fd;
+
+	if (mcc == NULL || mnc == NULL)
+		return -1;
+
+	fd = open(PROVIDER_DATABASE, O_RDONLY);
+	if (fd < 0)
+		return -1;
+
+	if (fstat(fd, &st) < 0) {
+		close(fd);
+		return -1;
+	}
+
+	map = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
+	if (map == NULL || map == MAP_FAILED) {
+		close(fd);
+		return -1;
+	}
+
+	data->match_mcc = mcc;
+	data->match_mnc = mnc;
+
+	parse_database(map, st.st_size, data);
+
+	munmap(map, st.st_size);
+
+	close(fd);
+
+	return 0;
+}
+
+static int gprs_provision(const char *mcc, const char *mnc,
+				const char *spn,
+				struct ofono_gprs_provision_data **settings,
+				int *count)
+{
+	int i;
+	struct parser_data *data;
+	*settings = NULL;
+
+	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'",
+			mcc, mnc, spn);
+
+	data = g_try_new0(struct parser_data, 1);
+	if (data == NULL)
+		return -ENOMEM;
+
+	lookup_apn(mcc, mnc, NULL, data);
+
+	*count = data->count;
+
+	DBG("settings count: %d", *count);
+
+	if (!*count) {
+		g_free(data);
+		return -ENOENT;
+	}
+
+	*settings = g_try_new0(struct ofono_gprs_provision_data, *count);
+	if (*settings == NULL)
+		return -ENOMEM;
+
+	for (i = 0; i < *count; i++) {
+		(*settings)[i] = *(data->settings[i]);
+
+		(*settings)[i].proto = OFONO_GPRS_PROTO_IP;
+		(*settings)[i].type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
+
+		DBG("Name: %s", (*settings)[i].name);
+		DBG("APN: %s", (*settings)[i].apn);
+		DBG("Username: %s", (*settings)[i].username);
+		DBG("Password: %s", (*settings)[i].password);
+	}
+
+	g_free(data);
+
+	return 0;
+}
+
+static struct ofono_gprs_provision_driver grps_provision_driver = {
+	.name		= "GPRS context provisioning",
+	.get_settings	= gprs_provision
+};
+
+static int gprs_provision_init(void)
+{
+	return ofono_gprs_provision_driver_register(&grps_provision_driver);
+}
+
+static void gprs_provision_exit(void)
+{
+	ofono_gprs_provision_driver_unregister(&grps_provision_driver);
+}
+
+OFONO_PLUGIN_DEFINE(gprs_provision, "GPRS Provisioning Plugin",
+			VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+			gprs_provision_init,
+			gprs_provision_exit)
-- 
1.7.1

---------------------------------------------------------------------
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.

Re: [RFC 1/1] GPRS Provisioning Plugin.

[Marcel Holtmann]

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

Hi Oleg,

>  plugins/gprs-provision.c |  290 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 290 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/gprs-provision.c

since this is mobile-broadband-provider-info specific, we better call
this plugin mobile-broadband-provider-info.c. Even if that is a horrible
long name ;)

And of course you need to get the autoconf/automake magic included as
well.

> diff --git a/plugins/gprs-provision.c b/plugins/gprs-provision.c
> new file mode 100644
> index 0000000..015ea7e
> --- /dev/null
> +++ b/plugins/gprs-provision.c
> @@ -0,0 +1,290 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
> + *
> + *  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 <config.h>
> +#endif
> +
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <glib.h>
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono/types.h>
> +#include <ofono/log.h>
> +#include <ofono/plugin.h>
> +#include <ofono/modem.h>
> +#include <ofono/gprs-provision.h>
> +
> +#define PROVISION_SETTINGS_MAX 10

Why do we bother with 10 here. Will we ever have more than 2 anyway?

> +struct parser_data {
> +	struct ofono_gprs_provision_data *settings[PROVISION_SETTINGS_MAX];

Using struct ofono_gprs_provision_data **settings seems better here.
Then the question on how many we allocate is up to the code itself. And
worst case it could even realloc that settings here.

> +	struct ofono_gprs_provision_data *current;
> +	int count;
> +	const char *match_mcc;
> +	const char *match_mnc;
> +	const char *name;
> +	const char *current_element;
> +	gboolean match_found;
> +};
> +
> +static void parse_element_start(GMarkupParseContext *context,
> +				const gchar *element_name,
> +				const gchar **attribute_names,
> +				const gchar **attribute_values,
> +				gpointer user_data, GError **error)
> +{
> +	struct parser_data *data = user_data;
> +
> +	if (g_str_equal(element_name, "name") == TRUE) {
> +		data->current_element = element_name;
> +		return;
> +	}
> +
> +	if (g_str_equal(element_name, "network-id") == TRUE) {
> +		const char *mcc = NULL, *mnc = NULL;
> +		int i;
> +
> +		for (i = 0; attribute_names[i]; i++) {
> +			if (g_str_equal(attribute_names[i], "mcc") == TRUE)
> +				mcc = attribute_values[i];
> +			if (g_str_equal(attribute_names[i], "mnc") == TRUE)
> +				mnc = attribute_values[i];
> +		}
> +
> +		if (g_strcmp0(mcc, data->match_mcc) == 0 &&
> +			g_strcmp0(mnc, data->match_mnc) == 0)
> +			data->match_found = TRUE;
> +	}
> +
> +	if (data->match_found == FALSE)
> +		return;
> +
> +	data->current_element = element_name;
> +
> +	if (g_str_equal(element_name, "apn") == TRUE) {
> +		const char *apn = NULL;
> +		int i;
> +
> +		for (i = 0; attribute_names[i]; i++) {
> +			if (g_str_equal(attribute_names[i], "value") == TRUE)
> +				apn = attribute_values[i];
> +		}
> +
> +		if (data->count >= PROVISION_SETTINGS_MAX) {
> +			data->match_found = FALSE;
> +			return;
> +		}
> +
> +		data->current = g_try_new0(struct ofono_gprs_provision_data, 1);
> +		if (data->current == NULL)
> +			return;
> +
> +		data->settings[data->count++] = data->current;

Now I question if we should bother with multiple settings anyway. In the
end we have 1 Internet context and 1 MMS context coming from this type
of database.

> +		if (data->current->apn == NULL)
> +			data->current->apn = g_strdup(apn);
> +
> +		if (data->current->name == NULL)
> +			data->current->name = g_strdup(data->name);
> +	}
> +}
> +
> +static void parse_element_end(GMarkupParseContext *context,
> +				const gchar *element_name,
> +				gpointer user_data, GError **error)
> +{
> +	struct parser_data *data = user_data;
> +
> +	if (g_str_equal(element_name, "gsm") == TRUE ||
> +		g_str_equal(element_name, "cdma") == TRUE) {
> +		data->match_found = FALSE;
> +		g_free((gpointer)data->name);
> +		data->name = NULL;
> +	}
> +}
> +
> +static void parse_text(GMarkupParseContext *context,
> +			const gchar *text, gsize text_len,
> +			gpointer user_data, GError **error)
> +{
> +	struct parser_data *data = user_data;
> +
> +	if (g_strcmp0(data->current_element, "name") == 0) {
> +		data->name = g_strndup(text, text_len);
> +		return;
> +	}
> +
> +	if (data->match_found == FALSE ||
> +		(data->current && data->current->apn == NULL))
> +		return;
> +
> +	if (g_strcmp0(data->current_element, "username") == 0)
> +		data->current->username = g_strndup(text, text_len);
> +	else if (g_strcmp0(data->current_element, "password") == 0)
> +		data->current->password = g_strndup(text, text_len);
> +}
> +
> +static void parser_error(GMarkupParseContext *context,
> +				GError *error, gpointer user_data)
> +{
> +	ofono_error("Error parsing %s: %s", PROVIDER_DATABASE, error->message);
> +}
> +
> +static const GMarkupParser parser = {
> +	parse_element_start,
> +	parse_element_end,
> +	parse_text,
> +	NULL,
> +	parser_error,
> +};
> +
> +static void parse_database(const char *data, ssize_t size,
> +	struct parser_data *parser_data)
> +{
> +	GMarkupParseContext *context;
> +	gboolean result;
> +
> +	parser_data->match_found = FALSE;
> +
> +	context = g_markup_parse_context_new(&parser,
> +						G_MARKUP_TREAT_CDATA_AS_TEXT,
> +						parser_data, NULL);
> +
> +	result = g_markup_parse_context_parse(context, data, size, NULL);
> +	if (result == TRUE)
> +		result = g_markup_parse_context_end_parse(context, NULL);
> +
> +	g_markup_parse_context_free(context);
> +}
> +
> +static int lookup_apn(const char *mcc, const char *mnc, const char *spn,
> +			struct parser_data *data)
> +{
> +	struct stat st;
> +	char *map;
> +	int fd;
> +
> +	if (mcc == NULL || mnc == NULL)
> +		return -1;
> +
> +	fd = open(PROVIDER_DATABASE, O_RDONLY);
> +	if (fd < 0)
> +		return -1;
> +
> +	if (fstat(fd, &st) < 0) {
> +		close(fd);
> +		return -1;
> +	}
> +
> +	map = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
> +	if (map == NULL || map == MAP_FAILED) {
> +		close(fd);
> +		return -1;
> +	}
> +
> +	data->match_mcc = mcc;
> +	data->match_mnc = mnc;
> +
> +	parse_database(map, st.st_size, data);
> +
> +	munmap(map, st.st_size);
> +
> +	close(fd);
> +
> +	return 0;
> +}
> +
> +static int gprs_provision(const char *mcc, const char *mnc,
> +				const char *spn,
> +				struct ofono_gprs_provision_data **settings,
> +				int *count)
> +{
> +	int i;
> +	struct parser_data *data;
> +	*settings = NULL;

In general we keep "int i" as last in the variable declaration list. And
we have an empty line between code and variable declaration.
> +
> +	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'",
> +			mcc, mnc, spn);

And such a debug should be before any code execution.

> +
> +	data = g_try_new0(struct parser_data, 1);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	lookup_apn(mcc, mnc, NULL, data);
> +
> +	*count = data->count;
> +
> +	DBG("settings count: %d", *count);
> +
> +	if (!*count) {
> +		g_free(data);
> +		return -ENOENT;
> +	}

I rather have that we check a return value of lookup_apn. And this check
is not making much sense. Since this a return value from our side. It
will be always provided by the core. And we have to return the count.

> +	*settings = g_try_new0(struct ofono_gprs_provision_data, *count);
> +	if (*settings == NULL)
> +		return -ENOMEM;

So this is pointless double allocation of the structs. You are already
carrying an array of ofono_gprs_provision_data. Why not use that one.

> +	for (i = 0; i < *count; i++) {
> +		(*settings)[i] = *(data->settings[i]);
> +
> +		(*settings)[i].proto = OFONO_GPRS_PROTO_IP;
> +		(*settings)[i].type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
> +
> +		DBG("Name: %s", (*settings)[i].name);
> +		DBG("APN: %s", (*settings)[i].apn);
> +		DBG("Username: %s", (*settings)[i].username);
> +		DBG("Password: %s", (*settings)[i].password);
> +	}
> +
> +	g_free(data);
> +
> +	return 0;
> +}
> +
> +static struct ofono_gprs_provision_driver grps_provision_driver = {
> +	.name		= "GPRS context provisioning",
> +	.get_settings	= gprs_provision
> +};
> +
> +static int gprs_provision_init(void)
> +{
> +	return ofono_gprs_provision_driver_register(&grps_provision_driver);
> +}
> +
> +static void gprs_provision_exit(void)
> +{
> +	ofono_gprs_provision_driver_unregister(&grps_provision_driver);
> +}
> +
> +OFONO_PLUGIN_DEFINE(gprs_provision, "GPRS Provisioning Plugin",
> +			VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
> +			gprs_provision_init,
> +			gprs_provision_exit)

Same here with the naming. It should be Mobile Broadband Provider Info.

Regards

Marcel

Re: [RFC 1/1] GPRS Provisioning Plugin.

[Oleg Zhurakivskyy]

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

Hello Marcel,

On 07/08/2011 07:55 PM, Marcel Holtmann wrote:
 > Now I question if we should bother with multiple settings anyway. In the
 > end we have 1 Internet context and 1 MMS context coming from this type
 > of database.

That's a good point. So, let's stick to max 2 settings for now. If there will be 
need in more than 2 settings, let's come back to this then.

Thanks for the comments. I will prepare and submit another patch.

Regards,
Oleg
---------------------------------------------------------------------
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.

Re: [RFC 1/1] GPRS Provisioning Plugin.

[Denis Kenzior]

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

Hi Oleg,

<snip>

> +static int gprs_provision(const char *mcc, const char *mnc,
> +				const char *spn,
> +				struct ofono_gprs_provision_data **settings,
> +				int *count)
> +{
> +	int i;
> +	struct parser_data *data;
> +	*settings = NULL;
> +
> +	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'",
> +			mcc, mnc, spn);
> +
> +	data = g_try_new0(struct parser_data, 1);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	lookup_apn(mcc, mnc, NULL, data);

What I wonder is what is the overhead of the XML parser right now (e.g.
how long it takes on reasonable embedded hardware).  If this takes a
while, then it might be better to pre-parse the mobile-provider-info db
during plugin initialization and not every time gprs_provision is
called.  Otherwise we run the risk of hanging the daemon while the
provision settings are being looked up.

> +
> +	*count = data->count;
> +
> +	DBG("settings count: %d", *count);
> +
> +	if (!*count) {
> +		g_free(data);
> +		return -ENOENT;
> +	}
> +
> +	*settings = g_try_new0(struct ofono_gprs_provision_data, *count);
> +	if (*settings == NULL)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < *count; i++) {
> +		(*settings)[i] = *(data->settings[i]);
> +
> +		(*settings)[i].proto = OFONO_GPRS_PROTO_IP;
> +		(*settings)[i].type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
> +
> +		DBG("Name: %s", (*settings)[i].name);
> +		DBG("APN: %s", (*settings)[i].apn);
> +		DBG("Username: %s", (*settings)[i].username);
> +		DBG("Password: %s", (*settings)[i].password);
> +	}
> +
> +	g_free(data);
> +
> +	return 0;
> +}
> +

<snip>

Regards,
-Denis

Re: [RFC 1/1] GPRS Provisioning Plugin.

[Oleg Zhurakivskyy]

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

Hello Denis,

On 07/08/2011 09:23 PM, Denis Kenzior wrote:
> What I wonder is what is the overhead of the XML parser right now (e.g.
> how long it takes on reasonable embedded hardware).  If this takes a
> while, then it might be better to pre-parse the mobile-provider-info db
> during plugin initialization and not every time gprs_provision is
> called.  Otherwise we run the risk of hanging the daemon while the
> provision settings are being looked up.

Thanks for the input. Yes, this might be the problem. I could try to 
test with N900 in order to get the idea of how long the parsing can 
take. Pre-parsing might be a good idea anyway, I will try to check how 
feasible it is.

Regards,
Oleg
---------------------------------------------------------------------
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.

Re: [RFC 1/1] GPRS Provisioning Plugin.

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

On Friday 08 July 2011 21:23:34 ext Denis Kenzior, you wrote:
> Hi Oleg,
> 
> <snip>
> 
> > +static int gprs_provision(const char *mcc, const char *mnc,
> > +				const char *spn,
> > +				struct ofono_gprs_provision_data **settings,
> > +				int *count)
> > +{
> > +	int i;
> > +	struct parser_data *data;
> > +	*settings = NULL;
> > +
> > +	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'",
> > +			mcc, mnc, spn);
> > +
> > +	data = g_try_new0(struct parser_data, 1);
> > +	if (data == NULL)
> > +		return -ENOMEM;
> > +
> > +	lookup_apn(mcc, mnc, NULL, data);
> 
> What I wonder is what is the overhead of the XML parser right now (e.g.
> how long it takes on reasonable embedded hardware).  If this takes a
> while, then it might be better to pre-parse the mobile-provider-info db
> during plugin initialization and not every time gprs_provision is
> called.  Otherwise we run the risk of hanging the daemon while the
> provision settings are being looked up.

Provisioning should be a fairly rare occurence. Preparsing the table into 
memory might be a waste of both CPU and memory in the common circumstances...

If you suspect the parser is slow (I doubt it, but I have not checked), then 
it might be better to just do it in a separate thread in the rare cases that 
you actually do need the data.

-- 
Rémi Denis-Courmont
http://www.remlab.net/

Re: [RFC 1/1] GPRS Provisioning Plugin.

[Denis Kenzior]

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

Hi Rémi,

On 07/11/2011 03:11 AM, Rémi Denis-Courmont wrote:
> On Friday 08 July 2011 21:23:34 ext Denis Kenzior, you wrote:
>> Hi Oleg,
>>
>> <snip>
>>
>>> +static int gprs_provision(const char *mcc, const char *mnc,
>>> +				const char *spn,
>>> +				struct ofono_gprs_provision_data **settings,
>>> +				int *count)
>>> +{
>>> +	int i;
>>> +	struct parser_data *data;
>>> +	*settings = NULL;
>>> +
>>> +	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'",
>>> +			mcc, mnc, spn);
>>> +
>>> +	data = g_try_new0(struct parser_data, 1);
>>> +	if (data == NULL)
>>> +		return -ENOMEM;
>>> +
>>> +	lookup_apn(mcc, mnc, NULL, data);
>>
>> What I wonder is what is the overhead of the XML parser right now (e.g.
>> how long it takes on reasonable embedded hardware).  If this takes a
>> while, then it might be better to pre-parse the mobile-provider-info db
>> during plugin initialization and not every time gprs_provision is
>> called.  Otherwise we run the risk of hanging the daemon while the
>> provision settings are being looked up.
> 
> Provisioning should be a fairly rare occurence. Preparsing the table into 
> memory might be a waste of both CPU and memory in the common circumstances...
> 

I do agree here, but it is the lesser evil than hanging the daemon for x
seconds.  However, first I'd like to know what the parsing speed is...

> If you suspect the parser is slow (I doubt it, but I have not checked), then 
> it might be better to just do it in a separate thread in the rare cases that 
> you actually do need the data.
> 

Separate thread will not help since the function has to return
synchronously.  I doubt the overhead of storing this information will be
big, but if it is then we can play other tricks like pre-parsing the db
into a quick-look-up file format at start-up, etc.

Regards,
-Denis

Re: [RFC 1/1] GPRS Provisioning Plugin.

[Oleg Zhurakivskyy]

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

Hello Denis,

On 07/08/2011 09:23 PM, Denis Kenzior wrote:
> What I wonder is what is the overhead of the XML parser right now (e.g.
> how long it takes on reasonable embedded hardware).  If this takes a
> while, then it might be better to pre-parse the mobile-provider-info db
> during plugin initialization and not every time gprs_provision is
> called.  Otherwise we run the risk of hanging the daemon while the
> provision settings are being looked up.

I have tested on N900 (MeeGo 1.2 Community Edition on microSDHC class 4 card), 
it takes 0.1 second to load and parse the mobile-provider-info database (154KB).

Is this a reasonable time or should we go with pre-parsing?

Regards,
Oleg

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

Re: [RFC 1/1] GPRS Provisioning Plugin.

[Denis Kenzior]

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

Hi Oleg,

On 07/13/2011 05:41 AM, Oleg Zhurakivskyy wrote:
> Hello Denis,
> 
> On 07/08/2011 09:23 PM, Denis Kenzior wrote:
>> What I wonder is what is the overhead of the XML parser right now (e.g.
>> how long it takes on reasonable embedded hardware).  If this takes a
>> while, then it might be better to pre-parse the mobile-provider-info db
>> during plugin initialization and not every time gprs_provision is
>> called.  Otherwise we run the risk of hanging the daemon while the
>> provision settings are being looked up.
> 
> I have tested on N900 (MeeGo 1.2 Community Edition on microSDHC class 4
> card), it takes 0.1 second to load and parse the mobile-provider-info
> database (154KB).
> 
> Is this a reasonable time or should we go with pre-parsing?

My rule of thumb is less than 300 ms is not really noticeable to the
user and anything up to 1 second is OK.  So in this case I'd say we
should be fine parsing on demand.

If we ever encounter performance problems we can revisit this decision.

Regards,
-Denis