Re: [PATCHv6 3/3] Mobile broadband provider info plugin

Re: [PATCHv6 3/3] Mobile broadband provider info plugin

[Denis Kenzior]

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

Hi Oleg,

On 09/14/2011 08:16 AM, Oleg Zhurakivskyy wrote:
> ---
>  plugins/mobile-broadband-provider-info.c |  804 ++++++++++++++++++++++++++++++
>  1 files changed, 804 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/mobile-broadband-provider-info.c
> 

After reviewing this patch I got the impression that you were still
making things just way too complicated and not taking full advantage of
the push/pop semantics.  So just for fun I wrote my own version in
plugins/mbpi.[ch] and reworked tools/lookup-apn to use that.  The code
seems to be valgrind clean and can handle duplicate detection, bail out
early, etc.

Can you please take a look and suggest improvements?  Once you think it
is good enough, I'd like you to rewrite this patch series to utilize
mbpi.c parser.

Regards,
-Denis

Re: [PATCHv6 3/3] Mobile broadband provider info plugin

[Denis Kenzior]

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

Hi Oleg,

On 09/20/2011 08:44 AM, Oleg Zhurakivskyy wrote:
> Hello Denis,
> 
> On 09/09/2011 07:58 AM, Denis Kenzior wrote:
>> After reviewing this patch I got the impression that you were still
>> making things just way too complicated and not taking full advantage of
>> the push/pop semantics.  So just for fun I wrote my own version in
>> plugins/mbpi.[ch] and reworked tools/lookup-apn to use that.  The code
>> seems to be valgrind clean and can handle duplicate detection, bail out
>> early, etc.
>>
>> Can you please take a look and suggest improvements?  Once you think it
>> is good enough, I'd like you to rewrite this patch series to utilize
>> mbpi.c parser.
> 
> Thanks, I have just looked. I am fine with this approach too.
> 
> One of the ideas of the implementation in PATCHv6 is to have the naming
> of tag_start() / tag_end() handlers to correspond to the actual tag names.
> 
> Can the same be achieved here? Can you please check PATCHv6 once again
> in order to check that idea?
> 

Nope, I don't think so.  The parser recursion only works when you parsed
the starting tag + any attributes, so you can't really use the subparser
idea in this way (e.g. network-id is particularly tricky).

The way I think about this is that the foo_start / foo_end handle all
elements that can be contained in <foo></foo>

> A few cosmetic issues:
> 
> - the naming of struct gsm_data, perhaps, parser_data or provider_info
> would fit?

I did think about this, but I wanted to convey the fact that we're
provisioning gsm providers only (e.g. contained in gsm tag).  However, I
really don't care here.

> - gsm_start() could be broken down into smaller functions?

Sure, this would partly accomplish your tag naming goal above.

> - Marcel wished the plugin naming "mobile-broadband-provider-info", even
> though it's a very long name. Shall we stick to that? I am fine with both.
> 

Naming the plugin provision.c would be better.  The full name is too long.

> Perhaps, some diagnostic output should be added too, in case anybody
> will need to troubleshoot it. For that, something like body_isprint()
> and ap_type_name() would be needed.
> 

You shouldn't need body_isprint unless you're doing something terribly
wrong.  The XML parser already ensures that the contents are valid UTF8.

> Are you fine with these?

Also, you might find that adding debugging might be tricky since this
code is intended to be used both from the plugin and the lookup-apn
tool.  I was too lazy to hook this up, but if you feel strongly about
it, then go ahead and add this.

Regards,
-Denis

[PATCHv6 0/3] Mobile broadband provider info plugin

[Oleg Zhurakivskyy]

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

Hello,

Please find the mobile broadband provider info plugin ("Internet Access Provider database" TODO item).

If enabled, the plugin reads mobile-broadband-provider-info database entries (PROVIDER_DATABASE) and returns GRPS context settings to oFono provisioning module.

Regards,
Oleg

Oleg Zhurakivskyy (3):
  Mobile broadband provider info plugin makefile changes
  Mobile broadband provider info plugin autoconf support
  Mobile broadband provider info plugin

 Makefile.am                              |    7 +
 configure.ac                             |   19 +-
 plugins/mobile-broadband-provider-info.c |  804 ++++++++++++++++++++++++++++++
 3 files changed, 824 insertions(+), 6 deletions(-)
 create mode 100644 plugins/mobile-broadband-provider-info.c

-- 
1.7.4.1

[PATCHv6 1/3] Mobile broadband provider info plugin makefile changes

[Oleg Zhurakivskyy]

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

---
 Makefile.am |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 832c8ac..d0fd056 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -376,6 +376,11 @@ builtin_libadd += @BLUEZ_LIBS@
 endif
 endif
 
+if PROVIDER_INFO
+builtin_modules += mobile_broadband_provider_info
+builtin_sources += plugins/mobile-broadband-provider-info.c
+endif
+
 if MAINTAINER_MODE
 builtin_modules += example_history
 builtin_sources += examples/history.c
@@ -383,8 +388,10 @@ builtin_sources += examples/history.c
 builtin_modules += example_nettime
 builtin_sources += examples/nettime.c
 
+if !PROVIDER_INFO
 builtin_modules += example_provision
 builtin_sources += examples/provision.c
+endif
 
 builtin_modules += example_emulator
 builtin_sources += examples/emulator.c
-- 
1.7.4.1

[PATCHv6 2/3] Mobile broadband provider info plugin autoconf support

[Oleg Zhurakivskyy]

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

---
 configure.ac |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index 9e62066..a224c8b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -197,14 +197,21 @@ AC_SUBST(BLUEZ_CFLAGS)
 AC_SUBST(BLUEZ_LIBS)
 AM_CONDITIONAL(BLUETOOTH, test "${enable_bluetooth}" != "no")
 
-AC_MSG_CHECKING([for mobile-broadband-provider-info])
-PKG_CHECK_EXISTS(mobile-broadband-provider-info,
-	_PKG_CONFIG(PROVIDER_DATABASE, [variable=database],
+AC_ARG_ENABLE(provider-info, AC_HELP_STRING([--enable-provider-info],
+				[enable mobile provider database support]),
+					[enable_provider_info=${enableval}])
+if (test "${enable_provider_info}" == "yes"); then
+	AC_MSG_CHECKING([for mobile-broadband-provider-info])
+	PKG_CHECK_EXISTS(mobile-broadband-provider-info,
+			_PKG_CONFIG(PROVIDER_DATABASE, [variable=database],
 					[mobile-broadband-provider-info])
-	AC_DEFINE_UNQUOTED(PROVIDER_DATABASE, "$pkg_cv_PROVIDER_DATABASE",
+		AC_DEFINE_UNQUOTED(PROVIDER_DATABASE,
+					"$pkg_cv_PROVIDER_DATABASE",
 					[Mobile provider database])
-	AC_MSG_RESULT([yes]),
-	AC_MSG_RESULT([no]))
+		AC_MSG_RESULT([yes]),
+		AC_MSG_ERROR(mobile-broadband-provider-info package is required))
+fi
+AM_CONDITIONAL(PROVIDER_INFO, test "${enable_provider_info}" == "yes")
 
 AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
 			[don't install configuration and data files]),
-- 
1.7.4.1

[PATCHv6 3/3] Mobile broadband provider info plugin

[Oleg Zhurakivskyy]

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

---
 plugins/mobile-broadband-provider-info.c |  804 ++++++++++++++++++++++++++++++
 1 files changed, 804 insertions(+), 0 deletions(-)
 create mode 100644 plugins/mobile-broadband-provider-info.c

diff --git a/plugins/mobile-broadband-provider-info.c b/plugins/mobile-broadband-provider-info.c
new file mode 100644
index 0000000..57a888f
--- /dev/null
+++ b/plugins/mobile-broadband-provider-info.c
@@ -0,0 +1,804 @@
+/*
+ *
+ *  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 MAX_APS	4
+
+#define _(x) case x: return (#x)
+
+enum provider_info_flags {
+	PROVIDER_INFO_FLAG_MATCH_FOUND	= 0x01,
+	PROVIDER_INFO_FLAG_AP_COMMIT	= 0x02,
+	PROVIDER_INFO_FLAG_INTERNET	= 0x04,
+	PROVIDER_INFO_FLAG_MMS		= 0x08,
+	PROVIDER_INFO_FLAG_WAP		= 0x10,
+};
+
+enum element_type {
+	ELEMENT_NONE,
+	ELEMENT_PROVIDER_NAME,
+	ELEMENT_SPN,
+	ELEMENT_GSM,
+	ELEMENT_NETWORK_ID,
+	ELEMENT_APN,
+	ELEMENT_USAGE,
+	ELEMENT_PLAN,
+	ELEMENT_NAME,
+	ELEMENT_USERNAME,
+	ELEMENT_PASSWORD,
+	ELEMENT_MAX,
+};
+
+enum parser_return_code {
+	PARSER_OK,
+	PARSER_ERROR,
+};
+
+enum parser_match_type {
+	PARSER_MATCH_ALL,
+	PARSER_MATCH_ONLY,
+};
+
+struct element_parser {
+	enum element_type type;
+	enum parser_match_type match_type;
+	GMarkupParser parser;
+};
+
+struct provider_info {
+	struct ofono_gprs_provision_data settings[MAX_APS];
+	struct ofono_gprs_provision_data *ap;
+	int ap_count;
+	char *spn;
+	const char *match_mcc;
+	const char *match_mnc;
+	const char *match_spn;
+	enum element_type element;
+	GQuark g_error_domain;
+	const struct element_parser *subparser;
+	guint8 flags;
+};
+
+static struct provider_info provider_info;
+
+static const char *ap_type_name(enum ofono_gprs_context_type type)
+{
+	switch (type) {
+		_(OFONO_GPRS_CONTEXT_TYPE_ANY);
+		_(OFONO_GPRS_CONTEXT_TYPE_INTERNET);
+		_(OFONO_GPRS_CONTEXT_TYPE_MMS);
+		_(OFONO_GPRS_CONTEXT_TYPE_WAP);
+		_(OFONO_GPRS_CONTEXT_TYPE_IMS);
+	}
+
+	return "OFONO_GPRS_CONTEXT_TYPE_<UNKNOWN>";
+}
+
+static enum element_type element_type(const gchar *element,
+					GMarkupParseContext *context)
+{
+	switch (element[0]) {
+	case 'a':
+		if (g_str_equal(element, "apn") == TRUE)
+			return ELEMENT_APN;
+		break;
+	case 'g':
+		if (g_str_equal(element, "gsm") == TRUE)
+			return ELEMENT_GSM;
+		break;
+	case 'n':
+		if (g_str_equal(element, "name") == TRUE) {
+			GSList *l = (GSList *)
+					g_markup_parse_context_get_element_stack
+					(context);
+
+			if (!g_slist_length(l))
+				break;
+
+			if (g_str_equal(g_slist_nth_data(l, 1),
+					"provider") == TRUE)
+				return ELEMENT_PROVIDER_NAME;
+			if (g_str_equal(g_slist_nth_data(l, 1),
+					"apn") == TRUE)
+				return ELEMENT_NAME;
+			break;
+		}
+		if (g_str_equal(element, "network-id") == TRUE)
+			return ELEMENT_NETWORK_ID;
+		break;
+	case 'p':
+		if (g_str_equal(element, "plan") == TRUE)
+			return ELEMENT_PLAN;
+		if (g_str_equal(element, "password") == TRUE)
+			return ELEMENT_PASSWORD;
+		break;
+	case 'u':
+		if (g_str_equal(element, "usage") == TRUE)
+			return ELEMENT_USAGE;
+		if (g_str_equal(element, "username") == TRUE)
+			return ELEMENT_USERNAME;
+	}
+
+	return ELEMENT_NONE;
+}
+
+static struct ofono_gprs_provision_data *ap_new(struct provider_info *data)
+{
+	if (data->ap_count < MAX_APS) {
+		struct ofono_gprs_provision_data *ap =
+			data->settings + data->ap_count++;
+
+		data->flags &= ~PROVIDER_INFO_FLAG_AP_COMMIT;
+
+		DBG("%p", ap);
+
+		return ap;
+
+	}
+
+	return NULL;
+}
+
+static void ap_value_free(char **value)
+{
+	g_free(*value);
+
+	*value = NULL;
+}
+
+static void ap_delete(struct provider_info *data)
+{
+	struct ofono_gprs_provision_data *ap = data->ap;
+
+	DBG("%p", ap);
+
+	ap_value_free(&ap->name);
+
+	ap_value_free(&ap->apn);
+
+	ap_value_free(&ap->username);
+
+	ap_value_free(&ap->password);
+
+	ap->type = OFONO_GPRS_CONTEXT_TYPE_ANY;
+
+	data->ap_count--;
+
+	data->ap--;
+}
+
+static void provider_info_free(struct provider_info *data)
+{
+	while (data->ap_count)
+		ap_delete(data);
+
+	g_free(data->spn);
+
+	data->spn = NULL;
+}
+
+static int ap_type_valid(struct ofono_gprs_provision_data *ap)
+{
+	switch (ap->type) {
+	case OFONO_GPRS_CONTEXT_TYPE_INTERNET:
+	case OFONO_GPRS_CONTEXT_TYPE_MMS:
+	case OFONO_GPRS_CONTEXT_TYPE_WAP:
+		return 1;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int ap_type_is_configured(struct provider_info *data,
+					enum ofono_gprs_context_type type)
+{
+	switch (type) {
+	case OFONO_GPRS_CONTEXT_TYPE_INTERNET:
+		return data->flags & PROVIDER_INFO_FLAG_INTERNET;
+	case OFONO_GPRS_CONTEXT_TYPE_MMS:
+		return data->flags & PROVIDER_INFO_FLAG_MMS;
+	case OFONO_GPRS_CONTEXT_TYPE_WAP:
+		return data->flags & PROVIDER_INFO_FLAG_WAP;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static void ap_type_mark_configured(struct provider_info *data,
+					enum ofono_gprs_context_type type)
+{
+	switch (type) {
+	case OFONO_GPRS_CONTEXT_TYPE_INTERNET:
+		data->flags |= PROVIDER_INFO_FLAG_INTERNET;
+		break;
+	case OFONO_GPRS_CONTEXT_TYPE_MMS:
+		data->flags |= PROVIDER_INFO_FLAG_MMS;
+		break;
+	case OFONO_GPRS_CONTEXT_TYPE_WAP:
+		data->flags |= PROVIDER_INFO_FLAG_WAP;
+		break;
+	default:
+		break;
+	}
+}
+
+static inline int body_isprint(const gchar *text, gsize text_len)
+{
+	int print = 0;
+	int i;
+
+	for (i = 0; i < (int)text_len; i++) {
+		if (g_ascii_isprint(text[i])) {
+			print = 1;
+			break;
+		}
+	}
+
+	return print;
+}
+
+static inline int subparser_push(enum element_type from,
+					GMarkupParseContext *context,
+					const gchar *element_name,
+					const gchar **attribute_names,
+					const gchar **attribute_values,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+	enum element_type type;
+
+	type = element_type(element_name, context);
+
+	if (type == from)
+		return 0;
+
+	if (!data->subparser[type].type ||
+			(data->subparser[type].match_type == PARSER_MATCH_ONLY
+			&& !(data->flags & PROVIDER_INFO_FLAG_MATCH_FOUND)))
+		return 1;
+
+	DBG("%s", element_name);
+
+	if (data->subparser[type].parser.start_element)
+		data->subparser[type].parser.start_element(context,
+							element_name,
+							attribute_names,
+							attribute_values,
+							user_data, error);
+
+	g_markup_parse_context_push(context, &data->subparser[type].parser,
+					user_data);
+	return 1;
+}
+
+static inline int subparser_pop(enum element_type from,
+				GMarkupParseContext *context,
+				const gchar *element_name,
+				gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+	enum element_type type;
+
+	type = element_type(element_name, context);
+
+	if (type == from)
+		return 0;
+
+	if (!data->subparser[type].type ||
+			(data->subparser[type].match_type == PARSER_MATCH_ONLY
+			&& !(data->flags & PROVIDER_INFO_FLAG_MATCH_FOUND)))
+		return 1;
+
+	if (data->subparser[type].parser.end_element)
+		data->subparser[type].parser.end_element(context, element_name,
+							user_data, error);
+
+	DBG("%s", element_name);
+
+	g_markup_parse_context_pop(context);
+
+	return 1;
+}
+
+static void network_id_start(GMarkupParseContext *context,
+					const gchar *element_name,
+					const gchar **attribute_names,
+					const gchar **attribute_values,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+	const char *mcc = NULL, *mnc = NULL;
+	int i;
+
+	DBG("%s", element_name);
+
+	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->flags |= PROVIDER_INFO_FLAG_MATCH_FOUND;
+}
+
+static void gsm_start(GMarkupParseContext *context,
+					const gchar *element_name,
+					const gchar **attribute_names,
+					const gchar **attribute_values,
+					gpointer user_data, GError **error)
+{
+	if (subparser_push(ELEMENT_GSM, context, element_name,
+				attribute_names, attribute_values,
+				user_data, error))
+		return;
+
+	DBG("%s", element_name);
+}
+
+static void gsm_end(GMarkupParseContext *context,
+					const gchar *element_name,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data;
+
+	if (subparser_pop(ELEMENT_GSM, context, element_name, user_data,
+				error))
+		return;
+
+	DBG("%s", element_name);
+
+	data = user_data;
+
+	if (data->flags & PROVIDER_INFO_FLAG_MATCH_FOUND)
+		g_set_error(error, data->g_error_domain, PARSER_OK,
+				"end parsing");
+}
+
+static void apn_start(GMarkupParseContext *context,
+					const gchar *element_name,
+					const gchar **attribute_names,
+					const gchar **attribute_values,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data;
+	int i;
+
+	if (subparser_push(ELEMENT_APN, context, element_name,
+				attribute_names, attribute_values,
+				user_data, error))
+		return;
+
+	DBG("%s", element_name);
+
+	data = user_data;
+
+	data->ap = ap_new(data);
+	if (data->ap == NULL) {
+		g_set_error(error, data->g_error_domain, PARSER_ERROR,
+				"MAX_APS %d reached, end parsing", MAX_APS);
+		return;
+	}
+
+	for (i = 0; attribute_names[i]; i++)
+		if (g_str_equal(attribute_names[i], "value") == TRUE) {
+			data->ap->apn = g_strdup(attribute_values[i]);
+			break;
+		}
+}
+
+static void apn_end(GMarkupParseContext *context,
+					const gchar *element_name,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data;
+
+	if (subparser_pop(ELEMENT_APN, context, element_name, user_data,
+				error))
+		return;
+
+	DBG("%s", element_name);
+
+	data = user_data;
+
+	if (!(data->flags & PROVIDER_INFO_FLAG_AP_COMMIT)) {
+		ap_delete(data);
+		return;
+	}
+
+	if (!ap_type_valid(data->ap)) {
+		DBG("AP '%s', type %s not present, AP ignored",
+			data->ap->apn, ap_type_name(data->ap->type));
+		ap_delete(data);
+		return;
+	}
+
+	ap_type_mark_configured(data, data->ap->type);
+}
+
+static void plan_start(GMarkupParseContext *context,
+					const gchar *element_name,
+					const gchar **attribute_names,
+					const gchar **attribute_values,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+	int i;
+
+	DBG("%s", element_name);
+
+	for (i = 0; attribute_names[i]; i++)
+		if (g_str_equal(attribute_names[i], "type") == TRUE) {
+			if (attribute_values[i] == NULL) {
+				DBG("AP '%s', plan is not present, AP ignored",
+					data->ap->apn);
+				break;
+			}
+
+			if (g_str_equal(attribute_values[i],
+					"postpaid") == TRUE ||
+					g_str_equal(attribute_values[i],
+						"prepaid") == TRUE) {
+				data->flags |= PROVIDER_INFO_FLAG_AP_COMMIT;
+				return;
+			}
+
+			DBG("AP '%s', plan '%s' is not supported, AP ignored",
+				data->ap->apn, attribute_values[i]);
+			break;
+		}
+}
+
+static void usage_start(GMarkupParseContext *context,
+					const gchar *element_name,
+					const gchar **attribute_names,
+					const gchar **attribute_values,
+					gpointer user_data, GError **error)
+{
+	const char *usage = NULL;
+	struct provider_info *data;
+	struct ofono_gprs_provision_data *ap;
+	enum ofono_gprs_context_type type;
+	int i;
+
+	DBG("%s", element_name);
+
+	data = user_data;
+	ap = data->ap;
+
+	for (i = 0; attribute_names[i]; i++)
+		if (g_str_equal(attribute_names[i], "type") == TRUE)
+			usage = attribute_values[i];
+
+	if (usage == NULL)
+		return;
+
+	if (g_str_equal(usage, "internet") == TRUE)
+		type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
+	else if (g_str_equal(usage, "mms") == TRUE)
+		type = OFONO_GPRS_CONTEXT_TYPE_MMS;
+	else if (g_str_equal(usage, "wap") == TRUE)
+		type = OFONO_GPRS_CONTEXT_TYPE_WAP;
+
+	if (ap_type_is_configured(data, type)) {
+		g_set_error(error, data->g_error_domain, PARSER_ERROR,
+				"AP '%s', multiple settings "
+				"for type '%s' found, fail provisioning",
+				ap->apn, ap_type_name(type));
+		ap_delete(data);
+		return;
+	}
+
+	ap->type = type;
+}
+
+static void element_text_set(GMarkupParseContext *context, const gchar *text,
+				gsize text_len, char **s)
+{
+	if (!body_isprint(text, text_len))
+		return;
+
+	DBG("%s '%*s'", g_markup_parse_context_get_element(context),
+		(int)text_len, text);
+
+	*s = g_strndup(text, text_len);
+}
+
+static void name_text(GMarkupParseContext *context, const gchar *text,
+			gsize text_len, gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+
+	element_text_set(context, text, text_len, &data->ap->name);
+}
+
+static void username_text(GMarkupParseContext *context,
+					const gchar *text, gsize text_len,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+
+	element_text_set(context, text, text_len, &data->ap->username);
+}
+
+static void password_text(GMarkupParseContext *context,
+					const gchar *text, gsize text_len,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+
+	element_text_set(context, text, text_len, &data->ap->password);
+}
+
+static void element_start(GMarkupParseContext *context,
+				const gchar *element_name,
+				const gchar **attribute_names,
+				const gchar **attribute_values,
+				gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+
+	if (subparser_push(ELEMENT_NONE, context, element_name, attribute_names,
+			attribute_values, user_data, error))
+		return;
+
+	if (data->flags & PROVIDER_INFO_FLAG_MATCH_FOUND)
+		DBG("%s", element_name);
+}
+
+static void element_end(GMarkupParseContext *context,
+					const gchar *element_name,
+					gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+
+	if (subparser_pop(ELEMENT_NONE, context, element_name, user_data,
+			error))
+		return;
+
+	if (data->flags & PROVIDER_INFO_FLAG_MATCH_FOUND)
+		DBG("%s", element_name);
+}
+
+static void parser_error(GMarkupParseContext *context,
+				GError *error, gpointer user_data)
+{
+	struct provider_info *data = user_data;
+	gint line_number;
+	gint char_number;
+
+	g_markup_parse_context_get_position(context, &line_number,
+						&char_number);
+
+	if (error->domain != data->g_error_domain) {
+		ofono_error("Error parsing %s, line %d, char %d: %s",
+				PROVIDER_DATABASE, line_number,
+				char_number, error->message);
+		return;
+	}
+
+	if (error->code == PARSER_OK)
+		return;
+
+	ofono_warn("%s, line %d, char %d: %s", PROVIDER_DATABASE, line_number,
+			char_number, error->message);
+
+	provider_info_free(data);
+}
+
+#define _SP(_type, _match, _start, _end, _text) {	\
+	ELEMENT_ ## _type,				\
+	_match,						\
+	{						\
+		_start,					\
+		_end,					\
+		_text,					\
+		NULL,					\
+		NULL,					\
+	}						\
+},
+
+static const struct element_parser subparser[ELEMENT_MAX] = {
+	_SP(NONE, PARSER_MATCH_ALL, NULL, NULL, NULL)
+	_SP(PROVIDER_NAME, PARSER_MATCH_ALL, NULL, NULL, NULL)
+	_SP(SPN, PARSER_MATCH_ALL, NULL, NULL, NULL)
+	_SP(GSM, PARSER_MATCH_ALL, gsm_start, gsm_end, NULL)
+	_SP(NETWORK_ID, PARSER_MATCH_ALL, network_id_start, NULL, NULL)
+	_SP(APN, PARSER_MATCH_ONLY, apn_start, apn_end, NULL)
+	_SP(USAGE, PARSER_MATCH_ONLY, usage_start, NULL, NULL)
+	_SP(PLAN, PARSER_MATCH_ONLY, plan_start, NULL, NULL)
+	_SP(NAME, PARSER_MATCH_ONLY, NULL, NULL, name_text)
+	_SP(USERNAME, PARSER_MATCH_ONLY, NULL, NULL, username_text)
+	_SP(PASSWORD, PARSER_MATCH_ONLY, NULL, NULL, password_text)
+};
+
+static const GMarkupParser parser = {
+	element_start,
+	element_end,
+	NULL,
+	NULL,
+	parser_error,
+};
+
+static void parse_database(const char *data, ssize_t size,
+				struct provider_info *provider_info)
+{
+	GMarkupParseContext *context;
+	gboolean result;
+
+	provider_info->flags = 0;
+
+	context = g_markup_parse_context_new(&parser,
+						G_MARKUP_TREAT_CDATA_AS_TEXT,
+						provider_info, 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 provider_info_lookup(struct provider_info *data, const char *mcc,
+				const char *mnc, const char *spn)
+{
+	struct stat st;
+	char *map;
+	int fd;
+
+	fd = open(PROVIDER_DATABASE, O_RDONLY);
+	if (fd < 0) {
+		ofono_error("Error: open(%s): %s", PROVIDER_DATABASE,
+				strerror(errno));
+		return -errno;
+	}
+
+	if (fstat(fd, &st) < 0) {
+		ofono_error("Error: fstat(%s): %s", PROVIDER_DATABASE,
+				strerror(errno));
+		close(fd);
+		return -errno;
+	}
+
+	map = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
+	if (map == MAP_FAILED) {
+		ofono_error("Error: mmap(%s): %s", PROVIDER_DATABASE,
+				strerror(errno));
+		close(fd);
+		return -errno;
+	}
+
+	memset(data, 0, sizeof(struct provider_info));
+
+	data->subparser = subparser;
+
+	data->match_mcc = mcc;
+	data->match_mnc = mnc;
+	data->match_spn = spn;
+
+	parse_database(map, st.st_size, data);
+
+	munmap(map, st.st_size);
+
+	close(fd);
+
+	return data->ap_count;
+}
+
+static int mobile_broadband_provider_info_get(const char *mcc,
+				const char *mnc, const char *spn,
+				struct ofono_gprs_provision_data **settings,
+				int *count)
+{
+	struct ofono_gprs_provision_data *aps;
+	int ap_count;
+	int i;
+
+	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'", mcc, mnc, spn);
+
+	ap_count = provider_info_lookup(&provider_info, mcc, mnc, spn);
+	if (ap_count <= 0) {
+		provider_info_free(&provider_info);
+		return -ENOENT;
+	}
+
+	DBG("Found %d APs", ap_count);
+
+	aps = g_try_malloc_n(ap_count,
+				sizeof(struct ofono_gprs_provision_data));
+	if (aps == NULL) {
+		ofono_error("Error provisioning APNs: memory exhausted");
+		provider_info_free(&provider_info);
+		return -ENOMEM;
+	}
+
+	memcpy(aps, provider_info.settings,
+		sizeof(struct ofono_gprs_provision_data) * ap_count);
+
+	memset(provider_info.settings, 0,
+		sizeof(struct ofono_gprs_provision_data) * ap_count);
+
+	*settings = aps;
+	*count = ap_count;
+
+	for (i = 0; i < ap_count; aps++, i++) {
+		DBG("Name: '%s'", aps->name);
+		DBG("APN: '%s'", aps->apn);
+		DBG("Type: %s", ap_type_name(aps->type));
+		DBG("Username: '%s'", aps->username);
+		DBG("Password: '%s'", aps->password);
+	}
+
+	provider_info_free(&provider_info);
+
+	return 0;
+}
+
+static struct ofono_gprs_provision_driver provision_driver = {
+	.name		= "Mobile Broadband Provider Info",
+	.get_settings	= mobile_broadband_provider_info_get
+};
+
+static int mobile_broadband_provider_info_init(void)
+{
+	provider_info.g_error_domain = g_quark_from_string(
+		provision_driver.name);
+
+	return ofono_gprs_provision_driver_register(&provision_driver);
+}
+
+static void mobile_broadband_provider_info_exit(void)
+{
+	ofono_gprs_provision_driver_unregister(&provision_driver);
+}
+
+OFONO_PLUGIN_DEFINE(mobile_broadband_provider_info,
+			"Mobile Broadband Provider Info Plugin",
+			VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+			mobile_broadband_provider_info_init,
+			mobile_broadband_provider_info_exit)
-- 
1.7.4.1

Re: [PATCHv6 3/3] Mobile broadband provider info plugin

[Oleg Zhurakivskyy]

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

Hello Denis,

On 09/09/2011 07:58 AM, Denis Kenzior wrote:
> After reviewing this patch I got the impression that you were still
> making things just way too complicated and not taking full advantage of
> the push/pop semantics.  So just for fun I wrote my own version in
> plugins/mbpi.[ch] and reworked tools/lookup-apn to use that.  The code
> seems to be valgrind clean and can handle duplicate detection, bail out
> early, etc.
>
> Can you please take a look and suggest improvements?  Once you think it
> is good enough, I'd like you to rewrite this patch series to utilize
> mbpi.c parser.

Thanks, I have just looked. I am fine with this approach too.

One of the ideas of the implementation in PATCHv6 is to have the naming of 
tag_start() / tag_end() handlers to correspond to the actual tag names.

Can the same be achieved here? Can you please check PATCHv6 once again in order 
to check that idea?

A few cosmetic issues:

- the naming of struct gsm_data, perhaps, parser_data or provider_info would fit?
- gsm_start() could be broken down into smaller functions?
- Marcel wished the plugin naming "mobile-broadband-provider-info", even though 
it's a very long name. Shall we stick to that? I am fine with both.

Perhaps, some diagnostic output should be added too, in case anybody will need 
to troubleshoot it. For that, something like body_isprint() and ap_type_name() 
would be needed.

Are you fine with these?

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