[PATCHv8 0/6] Provisioning plugin

[PATCHv8 0/6] Provisioning plugin

[Oleg Zhurakivskyy]

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

Hello,

Please find the provisioning 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 (6):
  plugins: Provisioning plugin autoconf support
  plugins: Provisioning plugin makefile changes
  plugins: mobile-broadband-provider-info parser changes
  plugins: mobile-broadband-provider-info parser changes
  plugins: Add provisioning plugin
  tools: lookup-apn update

 Makefile.am         |    7 +
 configure.ac        |   23 +++-
 plugins/mbpi.c      |  339 +++++++++++++++++++++++++++++++++------------------
 plugins/mbpi.h      |    4 +-
 plugins/provision.c |  125 +++++++++++++++++++
 tools/lookup-apn.c  |   32 +++---
 6 files changed, 389 insertions(+), 141 deletions(-)
 create mode 100644 plugins/provision.c

-- 
1.7.4.1

[PATCHv8 1/6] plugins: Provisioning plugin autoconf support

[Oleg Zhurakivskyy]

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

---
 configure.ac |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/configure.ac b/configure.ac
index 9e62066..36a6b73 100644
--- a/configure.ac
+++ b/configure.ac
@@ -197,14 +197,23 @@ 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(provision, AC_HELP_STRING([--enable-provision],
+				[enable GPRS context settings provisioning]),
+					[enable_provision=${enableval}])
+if (test "${enable_provision}" == "yes"); then
+	AC_MSG_CHECKING([for mobile-broadband-provider-info])
+	PKG_CHECK_EXISTS(mobile-broadband-provider-info,
+			_PKG_CONFIG(MBPI_DATABASE, [variable=database],
 					[mobile-broadband-provider-info])
-	AC_DEFINE_UNQUOTED(PROVIDER_DATABASE, "$pkg_cv_PROVIDER_DATABASE",
-					[Mobile provider database])
-	AC_MSG_RESULT([yes]),
-	AC_MSG_RESULT([no]))
+		AC_DEFINE_UNQUOTED(MBPI_DATABASE,
+					"$pkg_cv_MBPI_DATABASE",
+					[Provisioning database 
+					(mobile-broadband-provider-info 
+					package)])
+		AC_MSG_RESULT([yes]),
+		AC_MSG_ERROR(mobile-broadband-provider-info package is required))
+fi
+AM_CONDITIONAL(PROVISION, test "${enable_provision}" == "yes")
 
 AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
 			[don't install configuration and data files]),
-- 
1.7.4.1

[PATCHv8 2/6] plugins: Provisioning plugin makefile changes

[Oleg Zhurakivskyy]

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

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

diff --git a/Makefile.am b/Makefile.am
index 8771cb2..2704c75 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -381,6 +381,11 @@ builtin_libadd += @BLUEZ_LIBS@
 endif
 endif
 
+if PROVISION
+builtin_modules += provision
+builtin_sources += plugins/mbpi.c plugins/mbpi.h plugins/provision.c
+endif
+
 if MAINTAINER_MODE
 builtin_modules += example_history
 builtin_sources += examples/history.c
@@ -388,8 +393,10 @@ builtin_sources += examples/history.c
 builtin_modules += example_nettime
 builtin_sources += examples/nettime.c
 
+if !PROVISION
 builtin_modules += example_provision
 builtin_sources += examples/provision.c
+endif
 
 builtin_modules += example_emulator
 builtin_sources += examples/emulator.c
-- 
1.7.4.1

[PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes

[Oleg Zhurakivskyy]

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

---
 plugins/mbpi.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/plugins/mbpi.h b/plugins/mbpi.h
index fc9f738..bb6bfdc 100644
--- a/plugins/mbpi.h
+++ b/plugins/mbpi.h
@@ -19,7 +19,9 @@
  *
  */
 
-void mbpi_provision_data_free(struct ofono_gprs_provision_data *data);
+const char *mbpi_ap_type(enum ofono_gprs_context_type type);
+
+void mbpi_ap_free(struct ofono_gprs_provision_data *ap);
 
 GSList *mbpi_lookup(const char *mcc, const char *mnc,
 			gboolean allow_duplicates, GError **error);
-- 
1.7.4.1

[PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes

[Oleg Zhurakivskyy]

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

---
 plugins/mbpi.c |  339 ++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 221 insertions(+), 118 deletions(-)

diff --git a/plugins/mbpi.c b/plugins/mbpi.c
index 683ce03..0ae18d4 100644
--- a/plugins/mbpi.c
+++ b/plugins/mbpi.c
@@ -23,12 +23,12 @@
 #include <config.h>
 #endif
 
-#include <string.h>
-#include <fcntl.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
-#include <sys/types.h>
+
+#include <fcntl.h>
 #include <errno.h>
+#include <string.h>
 #include <unistd.h>
 
 #include <glib.h>
@@ -44,8 +44,11 @@
 
 #include "mbpi.h"
 
+#define _(x) case x: return (#x)
+
 enum MBPI_ERROR {
 	MBPI_ERROR_DUPLICATE,
+	MBPI_ERROR_ENOMEM,
 };
 
 struct gsm_data {
@@ -56,21 +59,75 @@ struct gsm_data {
 	gboolean allow_duplicates;
 };
 
+const char *mbpi_ap_type(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 GQuark mbpi_error_quark(void)
 {
 	return g_quark_from_static_string("ofono-mbpi-error-quark");
 }
 
-void mbpi_provision_data_free(struct ofono_gprs_provision_data *data)
+static void mbpi_g_set_error(GMarkupParseContext *context, GError **error,
+				GQuark domain, gint code, const gchar *fmt, ...)
 {
-	g_free(data->name);
-	g_free(data->apn);
-	g_free(data->username);
-	g_free(data->password);
-	g_free(data->message_proxy);
-	g_free(data->message_center);
-
-	g_free(data);
+	va_list ap;
+	gint line_number, char_number;
+
+	g_markup_parse_context_get_position(context, &line_number,
+						&char_number);
+	va_start(ap, fmt);
+
+	*error = g_error_new_valist(domain, code, fmt, ap);
+
+	va_end(ap);
+
+	g_prefix_error(error, "%s:%d ", MBPI_DATABASE, line_number);
+}
+
+static struct ofono_gprs_provision_data *mbpi_ap_new(const char *apn)
+{
+	struct ofono_gprs_provision_data *ap;
+	gsize len;
+
+	ap = g_try_new0(struct ofono_gprs_provision_data, 1);
+	if (ap == NULL)
+		return NULL;
+
+	len = strlen(apn) + 1;
+
+	ap->apn = g_try_new(char, len);
+	if (ap->apn == NULL) {
+		g_free(ap);
+		return NULL;
+	}
+
+	memcpy(ap->apn, apn, len);
+
+	ap->proto = OFONO_GPRS_PROTO_IP;
+
+	return ap;
+}
+
+void mbpi_ap_free(struct ofono_gprs_provision_data *ap)
+{
+	g_free(ap->name);
+	g_free(ap->apn);
+	g_free(ap->username);
+	g_free(ap->password);
+	g_free(ap->message_proxy);
+	g_free(ap->message_center);
+
+	g_free(ap);
 }
 
 static void text_handler(GMarkupParseContext *context,
@@ -79,7 +136,27 @@ static void text_handler(GMarkupParseContext *context,
 {
 	char **string = userdata;
 
-	*string = g_strndup(text, text_len);
+	if (*string != NULL) {
+		/*
+		 * For now, duplicate entries are just ignored. Since
+		 * this parser is also used by lookup-apn tool, ofono_warn()
+		 * can't be used here. As soon as the way is agreed,
+		 * it might be good to report this.
+		 */
+		return;
+	}
+
+	*string = g_try_new(char, text_len + 1);
+	if (*string == NULL) {
+		mbpi_g_set_error(context, error, mbpi_error_quark(),
+				MBPI_ERROR_ENOMEM,
+				"Can't allocate memory, memory exhausted");
+		return;
+	}
+
+	memcpy(*string, text, text_len);
+
+	(*string)[text_len] = '\0';
 }
 
 static const GMarkupParser text_parser = {
@@ -90,33 +167,34 @@ static const GMarkupParser text_parser = {
 	NULL,
 };
 
-static void usage_handler(GMarkupParseContext *context,
-				const gchar *text, gsize text_len,
-				gpointer userdata, GError **error)
+static void usage_start(GMarkupParseContext *context, const gchar *element_name,
+			const gchar **attribute_names,
+			const gchar **attribute_values,
+			gpointer userdata, GError **error)
 {
 	enum ofono_gprs_context_type *type = userdata;
+	const char *text = NULL;
+	int i;
+
+	for (i = 0; attribute_names[i]; i++)
+		if (g_str_equal(attribute_names[i], "type") == TRUE)
+			text = attribute_values[i];
 
-	if (strncmp(text, "internet", text_len) == 0)
+	if (text == NULL)
+		return;
+
+	if (strcmp(text, "internet") == 0)
 		*type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
-	else if (strncmp(text, "mms", text_len) == 0)
+	else if (strcmp(text, "mms") == 0)
 		*type = OFONO_GPRS_CONTEXT_TYPE_MMS;
-	else if (strncmp(text, "wap", text_len) == 0)
+	else if (strcmp(text, "wap") == 0)
 		*type = OFONO_GPRS_CONTEXT_TYPE_WAP;
 	else
-		g_set_error(error, G_MARKUP_ERROR,
-					G_MARKUP_ERROR_UNKNOWN_ATTRIBUTE,
-					"Unknown usage attribute: %.*s",
-					(int) text_len, text);
+		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
+				G_MARKUP_ERROR_UNKNOWN_ATTRIBUTE,
+				"Unknown usage attribute value: %s", text);
 }
 
-static const GMarkupParser usage_parser = {
-	NULL,
-	NULL,
-	usage_handler,
-	NULL,
-	NULL,
-};
-
 static void apn_start(GMarkupParseContext *context, const gchar *element_name,
 			const gchar **attribute_names,
 			const gchar **attribute_values,
@@ -133,8 +211,8 @@ static void apn_start(GMarkupParseContext *context, const gchar *element_name,
 		g_markup_parse_context_push(context, &text_parser,
 						&apn->password);
 	else if (g_str_equal(element_name, "usage"))
-		g_markup_parse_context_push(context, &usage_parser,
-						&apn->type);
+		usage_start(context, element_name, attribute_names,
+				attribute_values, &apn->type, error);
 }
 
 static void apn_end(GMarkupParseContext *context, const gchar *element_name,
@@ -142,8 +220,7 @@ static void apn_end(GMarkupParseContext *context, const gchar *element_name,
 {
 	if (g_str_equal(element_name, "name") ||
 			g_str_equal(element_name, "username") ||
-			g_str_equal(element_name, "password") ||
-			g_str_equal(element_name, "usage"))
+			g_str_equal(element_name, "password"))
 		g_markup_parse_context_pop(context);
 }
 
@@ -155,7 +232,7 @@ static void apn_error(GMarkupParseContext *context, GError *error,
 	 * be called.  So we always perform cleanup of the allocated
 	 * provision data
 	 */
-	mbpi_provision_data_free(userdata);
+	mbpi_ap_free(userdata);
 }
 
 static const GMarkupParser apn_parser = {
@@ -174,115 +251,138 @@ static const GMarkupParser skip_parser = {
 	NULL,
 };
 
-static void gsm_start(GMarkupParseContext *context, const gchar *element_name,
-			const gchar **attribute_names,
-			const gchar **attribute_values,
-			gpointer userdata, GError **error)
+static void network_id_handler(GMarkupParseContext *context,
+				struct gsm_data *gsm,
+				const gchar **attribute_names,
+				const gchar **attribute_values,
+				GError **error)
 {
-	struct gsm_data *gsm = userdata;
-
-	if (g_str_equal(element_name, "network-id")) {
-		const char *mcc = NULL, *mnc = NULL;
-		int i;
-
-		/*
-		 * For entries with multiple network-id elements, don't bother
-		 * searching if we already have a match
-		 */
-		if (gsm->match_found == TRUE)
-			return;
-
-		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];
-		}
+	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 (mcc == NULL) {
-			g_set_error(error, G_MARKUP_ERROR,
+	if (mcc == NULL) {
+		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
 					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
 					"Missing attribute: mcc");
-			return;
-		}
+		return;
+	}
 
-		if (mnc == NULL) {
-			g_set_error(error, G_MARKUP_ERROR,
+	if (mnc == NULL) {
+		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
 					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
 					"Missing attribute: mnc");
-			return;
-		}
+		return;
+	}
 
-		if (g_str_equal(mcc, gsm->match_mcc) &&
-				g_str_equal(mnc, gsm->match_mnc))
-			gsm->match_found = TRUE;
-	} else if (g_str_equal(element_name, "apn")) {
-		int i;
-		struct ofono_gprs_provision_data *pd;
-		const char *apn;
-
-		if (gsm->match_found == FALSE) {
-			g_markup_parse_context_push(context,
-							&skip_parser, NULL);
-			return;
-		}
+	if (g_str_equal(mcc, gsm->match_mcc) &&
+			g_str_equal(mnc, gsm->match_mnc))
+		gsm->match_found = TRUE;
+}
 
-		for (i = 0, apn = NULL; attribute_names[i]; i++) {
-			if (g_str_equal(attribute_names[i], "value") == FALSE)
-				continue;
+static void apn_handler(GMarkupParseContext *context, struct gsm_data *gsm,
+			const gchar **attribute_names,
+			const gchar **attribute_values,
+			GError **error)
+{
+	struct ofono_gprs_provision_data *ap;
+	const char *apn;
+	int i;
 
-			apn = attribute_values[i];
-			break;
-		}
+	if (gsm->match_found == FALSE) {
+		g_markup_parse_context_push(context, &skip_parser, NULL);
+		return;
+	}
+
+	for (i = 0, apn = NULL; attribute_names[i]; i++) {
+		if (g_str_equal(attribute_names[i], "value") == FALSE)
+			continue;
+
+		apn = attribute_values[i];
+		break;
+	}
 
-		if (apn == NULL) {
-			g_set_error(error, G_MARKUP_ERROR,
+	if (apn == NULL) {
+		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
 					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
 					"APN attribute missing");
-			return;
-		}
-
-		pd = g_new0(struct ofono_gprs_provision_data, 1);
-		pd->apn = g_strdup(apn);
-		pd->type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
-		pd->proto = OFONO_GPRS_PROTO_IP;
+		return;
+	}
 
-		g_markup_parse_context_push(context, &apn_parser, pd);
+	ap = mbpi_ap_new(apn);
+	if (ap == NULL) {
+		mbpi_g_set_error(context, error, mbpi_error_quark(),
+					MBPI_ERROR_ENOMEM,
+					"Can't allocate memory for APN: '%s', "
+					"memory exhausted", apn);
+		return;
 	}
+
+	g_markup_parse_context_push(context, &apn_parser, ap);
+}
+
+static void gsm_start(GMarkupParseContext *context, const gchar *element_name,
+			const gchar **attribute_names,
+			const gchar **attribute_values,
+			gpointer userdata, GError **error)
+{
+	if (g_str_equal(element_name, "network-id")) {
+		struct gsm_data *gsm = userdata;
+
+		/*
+		 * For entries with multiple network-id elements, don't bother
+		 * searching if we already have a match
+		 */
+		if (gsm->match_found == TRUE)
+			return;
+
+		network_id_handler(context, userdata, attribute_names,
+					attribute_values, error);
+	} else if (g_str_equal(element_name, "apn"))
+		apn_handler(context, userdata, attribute_names,
+				attribute_values, error);
 }
 
 static void gsm_end(GMarkupParseContext *context, const gchar *element_name,
 			gpointer userdata, GError **error)
 {
-	struct gsm_data *gsm = userdata;
+	struct gsm_data *gsm;
+	struct ofono_gprs_provision_data *ap;
 
-	if (g_str_equal(element_name, "apn")) {
-		struct ofono_gprs_provision_data *apn =
-					g_markup_parse_context_pop(context);
+	if (!g_str_equal(element_name, "apn"))
+		return;
 
-		if (apn == NULL)
-			return;
+	gsm = userdata;
 
-		if (gsm->allow_duplicates == FALSE) {
-			GSList *l;
+	ap = g_markup_parse_context_pop(context);
+	if (ap == NULL)
+		return;
 
-			for (l = gsm->apns; l; l = l->next) {
-				struct ofono_gprs_provision_data *pd = l->data;
+	if (gsm->allow_duplicates == FALSE) {
+		GSList *l;
 
-				if (pd->type != apn->type)
-					continue;
+		for (l = gsm->apns; l; l = l->next) {
+			struct ofono_gprs_provision_data *pd = l->data;
+
+			if (pd->type != ap->type)
+				continue;
 
-				g_set_error(error, mbpi_error_quark(),
+			mbpi_g_set_error(context, error, mbpi_error_quark(),
 						MBPI_ERROR_DUPLICATE,
 						"Duplicate context detected");
 
-				mbpi_provision_data_free(apn);
-				return;
-			}
+			mbpi_ap_free(ap);
+			return;
 		}
-
-		gsm->apns = g_slist_append(gsm->apns, apn);
 	}
+
+	gsm->apns = g_slist_append(gsm->apns, ap);
 }
 
 static const GMarkupParser gsm_parser = {
@@ -358,7 +458,8 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
 	if (fd < 0) {
 		g_set_error(error, G_FILE_ERROR,
 				g_file_error_from_errno(errno),
-				"open failed: %s", g_strerror(errno));
+				"open(%s) failed: %s", MBPI_DATABASE,
+				g_strerror(errno));
 		return NULL;
 	}
 
@@ -366,7 +467,8 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
 		close(fd);
 		g_set_error(error, G_FILE_ERROR,
 				g_file_error_from_errno(errno),
-				"fstat failed: %s", g_strerror(errno));
+				"fstat(%s) failed: %s", MBPI_DATABASE,
+				g_strerror(errno));
 		return NULL;
 	}
 
@@ -375,7 +477,8 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
 		close(fd);
 		g_set_error(error, G_FILE_ERROR,
 				g_file_error_from_errno(errno),
-				"mmap failed: %s", g_strerror(errno));
+				"mmap(%s) failed: %s", MBPI_DATABASE,
+				g_strerror(errno));
 		return NULL;
 	}
 
@@ -386,7 +489,7 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
 
 	if (mbpi_parse(db, st.st_size, &gsm, error) == FALSE) {
 		for (l = gsm.apns; l; l = l->next)
-			mbpi_provision_data_free(l->data);
+			mbpi_ap_free(l->data);
 
 		g_slist_free(gsm.apns);
 		gsm.apns = NULL;
-- 
1.7.4.1

[PATCHv8 5/6] plugins: Add provisioning plugin

[Oleg Zhurakivskyy]

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

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

diff --git a/plugins/provision.c b/plugins/provision.c
new file mode 100644
index 0000000..e00bab5
--- /dev/null
+++ b/plugins/provision.c
@@ -0,0 +1,125 @@
+/*
+ *
+ *  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 <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>
+
+#include "mbpi.h"
+
+static int provision_get_settings(const char *mcc, const char *mnc,
+				const char *spn,
+				struct ofono_gprs_provision_data **settings,
+				int *count)
+{
+	GSList *l;
+	GSList *apns;
+	GError *error = NULL;
+	int ap_count;
+	int i;
+
+	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'", mcc, mnc, spn);
+
+	apns = mbpi_lookup(mcc, mnc, FALSE, &error);
+	if (apns == NULL) {
+		if (error != NULL) {
+			ofono_error("%s", error->message);
+			g_error_free(error);
+		}
+
+		return -ENOENT;
+	}
+
+	ap_count = g_slist_length(apns);
+
+	DBG("Found %d APs", ap_count);
+
+	*settings = g_try_malloc_n(ap_count,
+				sizeof(struct ofono_gprs_provision_data));
+	if (*settings == NULL) {
+		ofono_error("Provisioning failed: %s", g_strerror(errno));
+
+		for (l = apns; l; l = l->next)
+			mbpi_ap_free(l->data);
+
+		g_slist_free(apns);
+
+		return -ENOMEM;
+	}
+
+	*count = ap_count;
+
+	for (l = apns, i = 0; l; l = l->next, i++) {
+		struct ofono_gprs_provision_data *ap = l->data;
+
+		DBG("Name: '%s'", ap->name);
+		DBG("APN: '%s'", ap->apn);
+		DBG("Type: %s", mbpi_ap_type(ap->type));
+		DBG("Username: '%s'", ap->username);
+		DBG("Password: '%s'", ap->password);
+
+		*(*settings + i) = *ap;
+
+		memset(*settings + i, 0,
+			sizeof(struct ofono_gprs_provision_data));
+		mbpi_ap_free(ap);
+	}
+
+	g_slist_free(apns);
+
+	return 0;
+}
+
+static struct ofono_gprs_provision_driver provision_driver = {
+	.name		= "Provisioning",
+	.get_settings	= provision_get_settings
+};
+
+static int provision_init(void)
+{
+	return ofono_gprs_provision_driver_register(&provision_driver);
+}
+
+static void provision_exit(void)
+{
+	ofono_gprs_provision_driver_unregister(&provision_driver);
+}
+
+OFONO_PLUGIN_DEFINE(provision, "Provisioning Plugin", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT,
+			provision_init, provision_exit)
-- 
1.7.4.1

[PATCHv8 6/6] tools: lookup-apn update

[Oleg Zhurakivskyy]

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

---
 tools/lookup-apn.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/tools/lookup-apn.c b/tools/lookup-apn.c
index 2969baf..c4cb928 100644
--- a/tools/lookup-apn.c
+++ b/tools/lookup-apn.c
@@ -24,12 +24,7 @@
 #endif
 
 #include <stdio.h>
-#include <fcntl.h>
-#include <unistd.h>
 #include <stdlib.h>
-#include <string.h>
-#include <sys/stat.h>
-#include <sys/mman.h>
 
 #include <glib.h>
 
@@ -39,31 +34,38 @@
 
 #include "plugins/mbpi.h"
 
-static void lookup_apn(const char *match_mcc, const char *match_mnc)
+static void lookup_apn(const char *mcc, const char *mnc)
 {
 	GSList *l;
 	GSList *apns;
 	GError *error = NULL;
 
-	printf("Searching for info for network: %s%s\n", match_mcc, match_mnc);
+	printf("Searching for info for network: %s%s\n", mcc, mnc);
 
-	apns = mbpi_lookup(match_mcc, match_mnc, TRUE, &error);
+	apns = mbpi_lookup(mcc, mnc, FALSE, &error);
 
 	if (apns == NULL) {
-		g_print("Lookup failed: %s\n", error->message);
+		if (error != NULL) {
+			g_printerr("Lookup failed: %s\n", error->message);
+			g_error_free(error);
+		}
+
 		return;
 	}
 
+	printf("Found %d APs\n", g_slist_length(apns));
+
 	for (l = apns; l; l = l->next) {
-		struct ofono_gprs_provision_data *apn = l->data;
+		struct ofono_gprs_provision_data *ap = l->data;
 
 		printf("\n");
-		printf("Name: %s\n", apn->name);
-		printf("APN: %s\n", apn->apn);
-		printf("Username: %s\n", apn->username);
-		printf("Password: %s\n", apn->password);
+		printf("Name: '%s'\n", ap->name);
+		printf("APN: '%s'\n", ap->apn);
+		printf("Type: %s\n", mbpi_ap_type(ap->type));
+		printf("Username: '%s'\n", ap->username);
+		printf("Password: '%s'\n", ap->password);
 
-		mbpi_provision_data_free(apn);
+		mbpi_ap_free(ap);
 	}
 
 	g_slist_free(apns);
-- 
1.7.4.1

Re: [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes

[Denis Kenzior]

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

Hi Oleg,

On 09/30/2011 08:31 AM, Oleg Zhurakivskyy wrote:
> ---
>  plugins/mbpi.h |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/plugins/mbpi.h b/plugins/mbpi.h
> index fc9f738..bb6bfdc 100644
> --- a/plugins/mbpi.h
> +++ b/plugins/mbpi.h
> @@ -19,7 +19,9 @@
>   *
>   */
>  
> -void mbpi_provision_data_free(struct ofono_gprs_provision_data *data);
> +const char *mbpi_ap_type(enum ofono_gprs_context_type type);
> +
> +void mbpi_ap_free(struct ofono_gprs_provision_data *ap);
>  
>  GSList *mbpi_lookup(const char *mcc, const char *mnc,
>  			gboolean allow_duplicates, GError **error);

This patch makes no logical sense.  If you want to add a new
mbpi_ap_type function, then send a patch against mbpi.h and mbpi.c
adding such a function.

If you want to rename mbpi_provision_data_free to mbpi_ap_free, then
send a patch against mbpi.h, mbpi.c and a separate patch against
tools/lookup-apn.c

The only time you should separate .h and .c patches are when you're
modifying public API in include/*

See HACKING document, the 'Submitting patches' section in particular.

Regards,
-Denis

Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes

[Denis Kenzior]

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

On 09/30/2011 08:31 AM, Oleg Zhurakivskyy wrote:
> ---
>  plugins/mbpi.c |  339 ++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 221 insertions(+), 118 deletions(-)
> 
> diff --git a/plugins/mbpi.c b/plugins/mbpi.c
> index 683ce03..0ae18d4 100644
> --- a/plugins/mbpi.c
> +++ b/plugins/mbpi.c
> @@ -23,12 +23,12 @@
>  #include <config.h>
>  #endif
>  
> -#include <string.h>
> -#include <fcntl.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
> -#include <sys/types.h>
> +
> +#include <fcntl.h>
>  #include <errno.h>
> +#include <string.h>
>  #include <unistd.h>
>  
>  #include <glib.h>
> @@ -44,8 +44,11 @@
>  
>  #include "mbpi.h"
>  
> +#define _(x) case x: return (#x)
> +
>  enum MBPI_ERROR {
>  	MBPI_ERROR_DUPLICATE,
> +	MBPI_ERROR_ENOMEM,

There's no point in introducing this one, just use
g_file_error_from_errno(ENOMEM)

>  };
>  
>  struct gsm_data {
> @@ -56,21 +59,75 @@ struct gsm_data {
>  	gboolean allow_duplicates;
>  };
>  
> +const char *mbpi_ap_type(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>";
> +}
> +

As previously mentioned, this part belongs in a separate patch

>  static GQuark mbpi_error_quark(void)
>  {
>  	return g_quark_from_static_string("ofono-mbpi-error-quark");
>  }
>  
> -void mbpi_provision_data_free(struct ofono_gprs_provision_data *data)
> +static void mbpi_g_set_error(GMarkupParseContext *context, GError **error,
> +				GQuark domain, gint code, const gchar *fmt, ...)
>  {
> -	g_free(data->name);
> -	g_free(data->apn);
> -	g_free(data->username);
> -	g_free(data->password);
> -	g_free(data->message_proxy);
> -	g_free(data->message_center);
> -
> -	g_free(data);
> +	va_list ap;
> +	gint line_number, char_number;
> +
> +	g_markup_parse_context_get_position(context, &line_number,
> +						&char_number);
> +	va_start(ap, fmt);
> +
> +	*error = g_error_new_valist(domain, code, fmt, ap);
> +
> +	va_end(ap);
> +
> +	g_prefix_error(error, "%s:%d ", MBPI_DATABASE, line_number);
> +}
> +
> +static struct ofono_gprs_provision_data *mbpi_ap_new(const char *apn)
> +{
> +	struct ofono_gprs_provision_data *ap;
> +	gsize len;
> +
> +	ap = g_try_new0(struct ofono_gprs_provision_data, 1);
> +	if (ap == NULL)
> +		return NULL;
> +
> +	len = strlen(apn) + 1;
> +
> +	ap->apn = g_try_new(char, len);
> +	if (ap->apn == NULL) {
> +		g_free(ap);
> +		return NULL;
> +	}
> +
> +	memcpy(ap->apn, apn, len);

g_strdup would be way cleaner here.

> +
> +	ap->proto = OFONO_GPRS_PROTO_IP;
> +
> +	return ap;
> +}
> +
> +void mbpi_ap_free(struct ofono_gprs_provision_data *ap)
> +{
> +	g_free(ap->name);
> +	g_free(ap->apn);
> +	g_free(ap->username);
> +	g_free(ap->password);
> +	g_free(ap->message_proxy);
> +	g_free(ap->message_center);
> +
> +	g_free(ap);
>  }
>  
>  static void text_handler(GMarkupParseContext *context,
> @@ -79,7 +136,27 @@ static void text_handler(GMarkupParseContext *context,
>  {
>  	char **string = userdata;
>  
> -	*string = g_strndup(text, text_len);
> +	if (*string != NULL) {
> +		/*
> +		 * For now, duplicate entries are just ignored. Since
> +		 * this parser is also used by lookup-apn tool, ofono_warn()
> +		 * can't be used here. As soon as the way is agreed,
> +		 * it might be good to report this.
> +		 */
> +		return;
> +	}

Are you sure you need this, under what circumstances would *string be
not NULL?

> +
> +	*string = g_try_new(char, text_len + 1);
> +	if (*string == NULL) {
> +		mbpi_g_set_error(context, error, mbpi_error_quark(),
> +				MBPI_ERROR_ENOMEM,
> +				"Can't allocate memory, memory exhausted");
> +		return;
> +	}
> +
> +	memcpy(*string, text, text_len);
> +
> +	(*string)[text_len] = '\0';

Why is g_strndup not good enough for you here?

>  }
>  
>  static const GMarkupParser text_parser = {
> @@ -90,33 +167,34 @@ static const GMarkupParser text_parser = {
>  	NULL,
>  };
>  
> -static void usage_handler(GMarkupParseContext *context,
> -				const gchar *text, gsize text_len,
> -				gpointer userdata, GError **error)
> +static void usage_start(GMarkupParseContext *context, const gchar *element_name,
> +			const gchar **attribute_names,
> +			const gchar **attribute_values,
> +			gpointer userdata, GError **error)
>  {
>  	enum ofono_gprs_context_type *type = userdata;
> +	const char *text = NULL;
> +	int i;
> +
> +	for (i = 0; attribute_names[i]; i++)
> +		if (g_str_equal(attribute_names[i], "type") == TRUE)
> +			text = attribute_values[i];
>  
> -	if (strncmp(text, "internet", text_len) == 0)
> +	if (text == NULL)
> +		return;
> +
> +	if (strcmp(text, "internet") == 0)
>  		*type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
> -	else if (strncmp(text, "mms", text_len) == 0)
> +	else if (strcmp(text, "mms") == 0)
>  		*type = OFONO_GPRS_CONTEXT_TYPE_MMS;
> -	else if (strncmp(text, "wap", text_len) == 0)
> +	else if (strcmp(text, "wap") == 0)
>  		*type = OFONO_GPRS_CONTEXT_TYPE_WAP;
>  	else
> -		g_set_error(error, G_MARKUP_ERROR,
> -					G_MARKUP_ERROR_UNKNOWN_ATTRIBUTE,
> -					"Unknown usage attribute: %.*s",
> -					(int) text_len, text);
> +		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
> +				G_MARKUP_ERROR_UNKNOWN_ATTRIBUTE,
> +				"Unknown usage attribute value: %s", text);
>  }
>  
> -static const GMarkupParser usage_parser = {
> -	NULL,
> -	NULL,
> -	usage_handler,
> -	NULL,
> -	NULL,
> -};
> -
>  static void apn_start(GMarkupParseContext *context, const gchar *element_name,
>  			const gchar **attribute_names,
>  			const gchar **attribute_values,
> @@ -133,8 +211,8 @@ static void apn_start(GMarkupParseContext *context, const gchar *element_name,
>  		g_markup_parse_context_push(context, &text_parser,
>  						&apn->password);
>  	else if (g_str_equal(element_name, "usage"))
> -		g_markup_parse_context_push(context, &usage_parser,
> -						&apn->type);
> +		usage_start(context, element_name, attribute_names,
> +				attribute_values, &apn->type, error);
>  }
>  
>  static void apn_end(GMarkupParseContext *context, const gchar *element_name,
> @@ -142,8 +220,7 @@ static void apn_end(GMarkupParseContext *context, const gchar *element_name,
>  {
>  	if (g_str_equal(element_name, "name") ||
>  			g_str_equal(element_name, "username") ||
> -			g_str_equal(element_name, "password") ||
> -			g_str_equal(element_name, "usage"))
> +			g_str_equal(element_name, "password"))
>  		g_markup_parse_context_pop(context);
>  }
>  
> @@ -155,7 +232,7 @@ static void apn_error(GMarkupParseContext *context, GError *error,
>  	 * be called.  So we always perform cleanup of the allocated
>  	 * provision data
>  	 */
> -	mbpi_provision_data_free(userdata);
> +	mbpi_ap_free(userdata);
>  }
>  
>  static const GMarkupParser apn_parser = {
> @@ -174,115 +251,138 @@ static const GMarkupParser skip_parser = {
>  	NULL,
>  };
>  
> -static void gsm_start(GMarkupParseContext *context, const gchar *element_name,
> -			const gchar **attribute_names,
> -			const gchar **attribute_values,
> -			gpointer userdata, GError **error)
> +static void network_id_handler(GMarkupParseContext *context,
> +				struct gsm_data *gsm,
> +				const gchar **attribute_names,
> +				const gchar **attribute_values,
> +				GError **error)
>  {
> -	struct gsm_data *gsm = userdata;
> -
> -	if (g_str_equal(element_name, "network-id")) {
> -		const char *mcc = NULL, *mnc = NULL;
> -		int i;
> -
> -		/*
> -		 * For entries with multiple network-id elements, don't bother
> -		 * searching if we already have a match
> -		 */
> -		if (gsm->match_found == TRUE)
> -			return;
> -
> -		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];
> -		}
> +	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 (mcc == NULL) {
> -			g_set_error(error, G_MARKUP_ERROR,
> +	if (mcc == NULL) {
> +		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
>  					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
>  					"Missing attribute: mcc");
> -			return;
> -		}
> +		return;
> +	}
>  
> -		if (mnc == NULL) {
> -			g_set_error(error, G_MARKUP_ERROR,
> +	if (mnc == NULL) {
> +		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
>  					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
>  					"Missing attribute: mnc");
> -			return;
> -		}
> +		return;
> +	}
>  
> -		if (g_str_equal(mcc, gsm->match_mcc) &&
> -				g_str_equal(mnc, gsm->match_mnc))
> -			gsm->match_found = TRUE;
> -	} else if (g_str_equal(element_name, "apn")) {
> -		int i;
> -		struct ofono_gprs_provision_data *pd;
> -		const char *apn;
> -
> -		if (gsm->match_found == FALSE) {
> -			g_markup_parse_context_push(context,
> -							&skip_parser, NULL);
> -			return;
> -		}
> +	if (g_str_equal(mcc, gsm->match_mcc) &&
> +			g_str_equal(mnc, gsm->match_mnc))
> +		gsm->match_found = TRUE;
> +}
>  
> -		for (i = 0, apn = NULL; attribute_names[i]; i++) {
> -			if (g_str_equal(attribute_names[i], "value") == FALSE)
> -				continue;
> +static void apn_handler(GMarkupParseContext *context, struct gsm_data *gsm,
> +			const gchar **attribute_names,
> +			const gchar **attribute_values,
> +			GError **error)
> +{
> +	struct ofono_gprs_provision_data *ap;
> +	const char *apn;
> +	int i;
>  
> -			apn = attribute_values[i];
> -			break;
> -		}
> +	if (gsm->match_found == FALSE) {
> +		g_markup_parse_context_push(context, &skip_parser, NULL);
> +		return;
> +	}
> +
> +	for (i = 0, apn = NULL; attribute_names[i]; i++) {
> +		if (g_str_equal(attribute_names[i], "value") == FALSE)
> +			continue;
> +
> +		apn = attribute_values[i];
> +		break;
> +	}
>  
> -		if (apn == NULL) {
> -			g_set_error(error, G_MARKUP_ERROR,
> +	if (apn == NULL) {
> +		mbpi_g_set_error(context, error, G_MARKUP_ERROR,
>  					G_MARKUP_ERROR_MISSING_ATTRIBUTE,
>  					"APN attribute missing");
> -			return;
> -		}
> -
> -		pd = g_new0(struct ofono_gprs_provision_data, 1);
> -		pd->apn = g_strdup(apn);
> -		pd->type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
> -		pd->proto = OFONO_GPRS_PROTO_IP;
> +		return;
> +	}
>  
> -		g_markup_parse_context_push(context, &apn_parser, pd);
> +	ap = mbpi_ap_new(apn);
> +	if (ap == NULL) {
> +		mbpi_g_set_error(context, error, mbpi_error_quark(),
> +					MBPI_ERROR_ENOMEM,
> +					"Can't allocate memory for APN: '%s', "
> +					"memory exhausted", apn);
> +		return;
>  	}
> +
> +	g_markup_parse_context_push(context, &apn_parser, ap);
> +}
> +
> +static void gsm_start(GMarkupParseContext *context, const gchar *element_name,
> +			const gchar **attribute_names,
> +			const gchar **attribute_values,
> +			gpointer userdata, GError **error)
> +{
> +	if (g_str_equal(element_name, "network-id")) {
> +		struct gsm_data *gsm = userdata;
> +
> +		/*
> +		 * For entries with multiple network-id elements, don't bother
> +		 * searching if we already have a match
> +		 */
> +		if (gsm->match_found == TRUE)
> +			return;
> +
> +		network_id_handler(context, userdata, attribute_names,
> +					attribute_values, error);
> +	} else if (g_str_equal(element_name, "apn"))
> +		apn_handler(context, userdata, attribute_names,
> +				attribute_values, error);
>  }

Separate patch please, something to the effect of mbpi: Split gsm_start
for readability

Add any new functionality after the refactoring patches.

>  
>  static void gsm_end(GMarkupParseContext *context, const gchar *element_name,
>  			gpointer userdata, GError **error)
>  {
> -	struct gsm_data *gsm = userdata;
> +	struct gsm_data *gsm;
> +	struct ofono_gprs_provision_data *ap;
>  
> -	if (g_str_equal(element_name, "apn")) {
> -		struct ofono_gprs_provision_data *apn =
> -					g_markup_parse_context_pop(context);
> +	if (!g_str_equal(element_name, "apn"))
> +		return;
>  
> -		if (apn == NULL)
> -			return;
> +	gsm = userdata;
>  
> -		if (gsm->allow_duplicates == FALSE) {
> -			GSList *l;
> +	ap = g_markup_parse_context_pop(context);
> +	if (ap == NULL)
> +		return;
>  
> -			for (l = gsm->apns; l; l = l->next) {
> -				struct ofono_gprs_provision_data *pd = l->data;
> +	if (gsm->allow_duplicates == FALSE) {
> +		GSList *l;
>  
> -				if (pd->type != apn->type)
> -					continue;
> +		for (l = gsm->apns; l; l = l->next) {
> +			struct ofono_gprs_provision_data *pd = l->data;
> +
> +			if (pd->type != ap->type)
> +				continue;
>  
> -				g_set_error(error, mbpi_error_quark(),
> +			mbpi_g_set_error(context, error, mbpi_error_quark(),
>  						MBPI_ERROR_DUPLICATE,
>  						"Duplicate context detected");
>  
> -				mbpi_provision_data_free(apn);
> -				return;
> -			}
> +			mbpi_ap_free(ap);
> +			return;
>  		}
> -
> -		gsm->apns = g_slist_append(gsm->apns, apn);
>  	}
> +
> +	gsm->apns = g_slist_append(gsm->apns, ap);
>  }

Fair enough, but separate patch please.  Something to the effect of:

mbpi: Reflow gsm_end()

>  
>  static const GMarkupParser gsm_parser = {
> @@ -358,7 +458,8 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
>  	if (fd < 0) {
>  		g_set_error(error, G_FILE_ERROR,
>  				g_file_error_from_errno(errno),
> -				"open failed: %s", g_strerror(errno));
> +				"open(%s) failed: %s", MBPI_DATABASE,
> +				g_strerror(errno));

If you want to add filename information to the error, then please group
this as a separate patch.

>  		return NULL;
>  	}
>  
> @@ -366,7 +467,8 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
>  		close(fd);
>  		g_set_error(error, G_FILE_ERROR,
>  				g_file_error_from_errno(errno),
> -				"fstat failed: %s", g_strerror(errno));
> +				"fstat(%s) failed: %s", MBPI_DATABASE,
> +				g_strerror(errno));

same comment as above

>  		return NULL;
>  	}
>  
> @@ -375,7 +477,8 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
>  		close(fd);
>  		g_set_error(error, G_FILE_ERROR,
>  				g_file_error_from_errno(errno),
> -				"mmap failed: %s", g_strerror(errno));
> +				"mmap(%s) failed: %s", MBPI_DATABASE,
> +				g_strerror(errno));

same comment as above

>  		return NULL;
>  	}
>  
> @@ -386,7 +489,7 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
>  
>  	if (mbpi_parse(db, st.st_size, &gsm, error) == FALSE) {
>  		for (l = gsm.apns; l; l = l->next)
> -			mbpi_provision_data_free(l->data);
> +			mbpi_ap_free(l->data);

See my comments to the previous patch, this should be in a separate patch.

>  
>  		g_slist_free(gsm.apns);
>  		gsm.apns = NULL;

Please fix all the comments above, then I can have another look.  It is
too hard for me to tell what exactly you're changing when everything is
in one huge patch.

Regards,
-Denis

Re: [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes

[Oleg Zhurakivskyy]

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

Hello Denis,

On 09/30/2011 05:57 PM, Denis Kenzior wrote:
> This patch makes no logical sense.  If you want to add a new
> mbpi_ap_type function, then send a patch against mbpi.h and mbpi.c
> adding such a function.
>
> If you want to rename mbpi_provision_data_free to mbpi_ap_free, then
> send a patch against mbpi.h, mbpi.c and a separate patch against
> tools/lookup-apn.c
>
> The only time you should separate .h and .c patches are when you're
> modifying public API in include/*
>
> See HACKING document, the 'Submitting patches' section in particular.

Thanks for the info, indeed splitting of patches per directory is explained 
there. However, 'Submitting patches' is actually silent about the rest.

I will prepare and send another patch set.

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

Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes

[Oleg Zhurakivskyy]

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

Hello Denis,

On 09/30/2011 06:20 PM, Denis Kenzior wrote:
>>   enum MBPI_ERROR {
>>   	MBPI_ERROR_DUPLICATE,
>> +	MBPI_ERROR_ENOMEM,
>
> There's no point in introducing this one, just use
> g_file_error_from_errno(ENOMEM)

OK.

>> +const char *mbpi_ap_type(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>";
>> +}
>> +
>
> As previously mentioned, this part belongs in a separate patch

OK.

>> +static struct ofono_gprs_provision_data *mbpi_ap_new(const char *apn)
>> +{
>> +	struct ofono_gprs_provision_data *ap;
>> +	gsize len;
>> +
>> +	ap = g_try_new0(struct ofono_gprs_provision_data, 1);
>> +	if (ap == NULL)
>> +		return NULL;
>> +
>> +	len = strlen(apn) + 1;
>> +
>> +	ap->apn = g_try_new(char, len);
>> +	if (ap->apn == NULL) {
>> +		g_free(ap);
>> +		return NULL;
>> +	}
>> +
>> +	memcpy(ap->apn, apn, len);
>
> g_strdup would be way cleaner here.

g_strdup uses g_new, which aborts the program on failure. Since we gracefully 
handle access point allocation error with g_set_error(), would make sense?

>>   static void text_handler(GMarkupParseContext *context,
>> @@ -79,7 +136,27 @@ static void text_handler(GMarkupParseContext *context,
>>   {
>>   	char **string = userdata;
>>
>> -	*string = g_strndup(text, text_len);
>> +	if (*string != NULL) {
>> +		/*
>> +		 * For now, duplicate entries are just ignored. Since
>> +		 * this parser is also used by lookup-apn tool, ofono_warn()
>> +		 * can't be used here. As soon as the way is agreed,
>> +		 * it might be good to report this.
>> +		 */
>> +		return;
>> +	}
>
> Are you sure you need this, under what circumstances would *string be
> not NULL?

In case there are multiple "name", "username", "password" entries per APN. In 
theory this can't happen with the proper validation of the database, in practice 
users can edit it manually and use without the validation, which would result in 
the memory leak. Does this make sense?

>> +	*string = g_try_new(char, text_len + 1);
>> +	if (*string == NULL) {
>> +		mbpi_g_set_error(context, error, mbpi_error_quark(),
>> +				MBPI_ERROR_ENOMEM,
>> +				"Can't allocate memory, memory exhausted");
>> +		return;
>> +	}
>> +
>> +	memcpy(*string, text, text_len);
>> +
>> +	(*string)[text_len] = '\0';
>
> Why is g_strndup not good enough for you here?

g_strndup also uses g_new, which aborts program on failure, just in order to 
handle the memory allocation failure consistently gracefully.

However, silently ignoring the memory allocation failure here is probably not a 
good idea either, since this would lead to the incorrect settings and will 
confuse the user? Should we handle it gracefully here, bail out of parser and 
report the error?

> Separate patch please, something to the effect of mbpi: Split gsm_start
> for readability
>
> Add any new functionality after the refactoring patches.

OK.

> Fair enough, but separate patch please.  Something to the effect of:
>
> mbpi: Reflow gsm_end()

OK.

>>   static const GMarkupParser gsm_parser = {
>> @@ -358,7 +458,8 @@ GSList *mbpi_lookup(const char *mcc, const char *mnc,
>>   	if (fd<  0) {
>>   		g_set_error(error, G_FILE_ERROR,
>>   				g_file_error_from_errno(errno),
>> -				"open failed: %s", g_strerror(errno));
>> +				"open(%s) failed: %s", MBPI_DATABASE,
>> +				g_strerror(errno));
>
> If you want to add filename information to the error, then please group
> this as a separate patch.

OK.

> Please fix all the comments above, then I can have another look.  It is
> too hard for me to tell what exactly you're changing when everything is
> in one huge patch.

Thanks for the comments, I will prepare another set.

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

Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes

[Denis Kenzior]

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

Hi Oleg,

>>> +static struct ofono_gprs_provision_data *mbpi_ap_new(const char *apn)
>>> +{
>>> +    struct ofono_gprs_provision_data *ap;
>>> +    gsize len;
>>> +
>>> +    ap = g_try_new0(struct ofono_gprs_provision_data, 1);
>>> +    if (ap == NULL)
>>> +        return NULL;
>>> +
>>> +    len = strlen(apn) + 1;
>>> +
>>> +    ap->apn = g_try_new(char, len);
>>> +    if (ap->apn == NULL) {
>>> +        g_free(ap);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    memcpy(ap->apn, apn, len);
>>
>> g_strdup would be way cleaner here.
> 
> g_strdup uses g_new, which aborts the program on failure. Since we
> gracefully handle access point allocation error with g_set_error(),
> would make sense?
> 

While you're strictly right, in practice this is not something to worry
about.  Our convention is to ignore the possibility of failure on small
allocations, these can't really happen on Linux anyway.  The OOM killer
will kill something before that happens, likely a system daemon that is
even more important than oFono ;)

The original code was actually fine...  If you want to be paranoid then
checking that the length of the APN is sane (e.g. within
OFONO_GPRS_MAX_APN_LENGTH) would be enough.

As a rule of thumb the only time you should worry about memory
allocations is when you're allocating more than a few pages worth of
memory.

>>>   static void text_handler(GMarkupParseContext *context,
>>> @@ -79,7 +136,27 @@ static void text_handler(GMarkupParseContext
>>> *context,
>>>   {
>>>       char **string = userdata;
>>>
>>> -    *string = g_strndup(text, text_len);
>>> +    if (*string != NULL) {
>>> +        /*
>>> +         * For now, duplicate entries are just ignored. Since
>>> +         * this parser is also used by lookup-apn tool, ofono_warn()
>>> +         * can't be used here. As soon as the way is agreed,
>>> +         * it might be good to report this.
>>> +         */
>>> +        return;
>>> +    }
>>
>> Are you sure you need this, under what circumstances would *string be
>> not NULL?
> 
> In case there are multiple "name", "username", "password" entries per
> APN. In theory this can't happen with the proper validation of the
> database, in practice users can edit it manually and use without the
> validation, which would result in the memory leak. Does this make sense?
> 

I don't think it does, no user should be able to edit the provisioning
database.  oFono is running as root and expects this to be readable by
root only.  If you really want to handle this case then returning an
error (since that is what it is) would be way better.

>>> +    *string = g_try_new(char, text_len + 1);
>>> +    if (*string == NULL) {
>>> +        mbpi_g_set_error(context, error, mbpi_error_quark(),
>>> +                MBPI_ERROR_ENOMEM,
>>> +                "Can't allocate memory, memory exhausted");
>>> +        return;
>>> +    }
>>> +
>>> +    memcpy(*string, text, text_len);
>>> +
>>> +    (*string)[text_len] = '\0';
>>
>> Why is g_strndup not good enough for you here?
> 
> g_strndup also uses g_new, which aborts program on failure, just in
> order to handle the memory allocation failure consistently gracefully.
> 
> However, silently ignoring the memory allocation failure here is
> probably not a good idea either, since this would lead to the incorrect
> settings and will confuse the user? Should we handle it gracefully here,
> bail out of parser and report the error?
> 

See above, checking that a string is not excessively long is enough.
Besides, g_set_error uses g_strdup anyway, so you're still going to
abort.  Simply put, don't over-complicate things.

Regards,
-Denis

Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes

[Oleg Zhurakivskyy]

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

Hello Denis,

On 10/04/2011 05:05 PM, Denis Kenzior wrote:
> While you're strictly right, in practice this is not something to worry
> about.  Our convention is to ignore the possibility of failure on small
> allocations, these can't really happen on Linux anyway.  The OOM killer
> will kill something before that happens, likely a system daemon that is
> even more important than oFono ;)
>
> As a rule of thumb the only time you should worry about memory
> allocations is when you're allocating more than a few pages worth of
> memory.

OK. Thanks for the clarification, I will follow this approach.

>> In case there are multiple "name", "username", "password" entries per
>> APN. In theory this can't happen with the proper validation of the
>> database, in practice users can edit it manually and use without the
>> validation, which would result in the memory leak. Does this make sense?
>
> I don't think it does, no user should be able to edit the provisioning
> database.  oFono is running as root and expects this to be readable by
> root only.  If you really want to handle this case then returning an
> error (since that is what it is) would be way better.

OK, let's stick to the original approach here.

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