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

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

[Denis Kenzior]

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

Hi Oleg,

On 08/30/2011 07:56 AM, Oleg Zhurakivskyy wrote:
> ---
>  plugins/mobile-broadband-provider-info.c |  695 ++++++++++++++++++++++++++++++
>  1 files changed, 695 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..f8c898e
> --- /dev/null
> +++ b/plugins/mobile-broadband-provider-info.c
> @@ -0,0 +1,695 @@
> +/*
> + *
> + *  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 ap_type_configured_flags {
> +	AP_TYPE_CONFIGURED_FLAG_INTERNET	= 0x1,
> +	AP_TYPE_CONFIGURED_FLAG_MMS		= 0x2,
> +	AP_TYPE_CONFIGURED_FLAG_WAP		= 0x4,
> +};
> +
> +enum element_type {
> +	ELEMENT_PROVIDER,
> +	ELEMENT_SPN,
> +	ELEMENT_NETWORK_ID,
> +	ELEMENT_GSM,
> +	ELEMENT_APN,
> +	ELEMENT_USAGE,
> +	ELEMENT_PLAN,
> +	ELEMENT_NAME,
> +	ELEMENT_USERNAME,
> +	ELEMENT_PASSWORD,
> +	ELEMENT_NONE,
> +};
> +
> +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;
> +	gboolean match_found;
> +	gboolean ap_ignore;
> +	gboolean provisioning_fail;

I'm not particularly happy with this approach, can't we simply bail out
when we detect this condition?  Perhaps we should just invent our own
GError and report it when this condition occurs in e.g. start_element /
end_element functions.

> +	guint8 ap_type_configured;
> +};
> +
> +static struct provider_info provider_info;
> +
> +static const char *element_type_name(enum element_type element)
> +{
> +	switch (element) {
> +		_(ELEMENT_PROVIDER);
> +		_(ELEMENT_SPN);
> +		_(ELEMENT_NETWORK_ID);
> +		_(ELEMENT_GSM);
> +		_(ELEMENT_APN);
> +		_(ELEMENT_USAGE);
> +		_(ELEMENT_PLAN);
> +		_(ELEMENT_NAME);
> +		_(ELEMENT_USERNAME);
> +		_(ELEMENT_PASSWORD);
> +		_(ELEMENT_NONE);
> +	}
> +
> +	return "ELEMENT_<UNKNOWN>";
> +}
> +
> +static const char *ap_type_name(enum ofono_gprs_context_type ap_type)
> +{
> +	switch (ap_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)
> +{
> +	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)
> +			return ELEMENT_NAME;
> +		if (g_str_equal(element, "network-id") == TRUE)
> +			return ELEMENT_NETWORK_ID;
> +		break;
> +	case 'p':
> +		if (g_str_equal(element, "provider") == TRUE)
> +			return ELEMENT_PROVIDER;
> +		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;
> +}
> +

I like this optimization, but have you considered using
g_markup_parse_context_push / pop to simplify the logic even more?

> +static void element_network_id_parse(struct provider_info *data,
> +					const gchar **attribute_names,
> +					const gchar **attribute_values)
> +{
> +	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) {
> +		if (data->match_spn == NULL) {
> +			data->match_found = TRUE;
> +			return;
> +		}
> +
> +		if (g_strcmp0(data->spn, data->match_spn) == 0)
> +			data->match_found = TRUE;
> +	}

I just double checked serviceproviders.2.dtd and can't even find an
entry for the SPN.  I mentioned this before, but you can't treat the
provider name the same as the SPN.  The xml database is simply not setup
this way and the results you will get will be wrong.  Can we please drop
this check for now?

> +}
> +
> +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++;
> +
> +		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 element_apn_parse(struct provider_info *data,
> +				const gchar **attribute_names,
> +				const gchar **attribute_values)
> +{
> +	int i;
> +
> +	data->ap = ap_new(data);
> +	if (data->ap == NULL) {
> +		DBG("MAX_APS %d reached, end parsing", MAX_APS);
> +		data->match_found = FALSE;
> +		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 element_usage_parse(struct provider_info *data,
> +				const gchar **attribute_names,
> +				const gchar **attribute_values)
> +{
> +	struct ofono_gprs_provision_data *ap = data->ap;
> +	const char *usage = NULL;
> +	int i;
> +
> +	for (i = 0; attribute_names[i]; i++) {
> +		if (g_str_equal(attribute_names[i], "type") == TRUE)
> +			usage = attribute_values[i];
> +	}
> +
> +	if (usage == NULL) {
> +		DBG("AP '%s', no type, AP ignored", ap->apn);
> +		data->ap_ignore = TRUE;
> +		return;
> +	}
> +
> +	if (g_str_equal(usage, "internet") == TRUE)
> +		ap->type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
> +	else if (g_str_equal(usage, "mms") == TRUE)
> +		ap->type = OFONO_GPRS_CONTEXT_TYPE_MMS;
> +	else if (g_str_equal(usage, "wap") == TRUE)
> +		ap->type = OFONO_GPRS_CONTEXT_TYPE_WAP;
> +}
> +
> +static void element_plan_parse(struct provider_info *data,
> +				const gchar **attribute_names,
> +				const gchar **attribute_values)
> +{
> +	int i;
> +
> +	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)
> +				return;
> +
> +			DBG("AP '%s', plan '%s' is not supported, AP ignored",
> +				data->ap->apn, attribute_values[i]);
> +			break;
> +		}
> +
> +	data->ap_ignore = TRUE;
> +}
> +
> +static void element_start_parse(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 prev = data->element;
> +
> +	data->element = element_type(element_name);
> +
> +	if (prev == ELEMENT_PROVIDER && data->element == ELEMENT_NAME) {
> +		data->element = ELEMENT_SPN;
> +		g_free(data->spn);
> +		data->spn = NULL;
> +		data->match_found = TRUE;
> +		return;
> +	}
> +
> +	if (data->element == ELEMENT_NETWORK_ID)
> +		element_network_id_parse(data, attribute_names,
> +						attribute_values);
> +
> +	if (data->match_found == FALSE || data->ap_ignore == TRUE)
> +		return;
> +
> +	DBG("%s: '%s'", element_type_name(data->element), element_name);
> +
> +	switch (data->element) {
> +	case ELEMENT_APN:
> +		element_apn_parse(data, attribute_names, attribute_values);
> +		break;
> +	case ELEMENT_USAGE:
> +		element_usage_parse(data, attribute_names, attribute_values);
> +		break;
> +	case ELEMENT_PLAN:
> +		element_plan_parse(data, attribute_names, attribute_values);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static gchar *body_text_parse(const gchar *text, gsize text_len)
> +{
> +	gchar *body = g_strndup(text, text_len);
> +	gchar *print = NULL;
> +	int i;
> +
> +	for (i = 0; body && body[i]; i++) {
> +		if (g_ascii_isprint(body[i])) {
> +			print = g_strescape(body, NULL);
> +			break;
> +		}
> +	}
> +
> +	g_free(body);
> +
> +	return print;

What in the world is this doing?  Since the body can contain UTF8
characters, I don't think you should be escaping anything here.  If
you're trying to validate utf8, then you might need to do something else.

> +}
> +
> +static char *body_to_hex(const gchar *text, gsize text_len)
> +{
> +	char *hex;
> +	guint i, j;
> +
> +	hex = g_try_malloc(text_len * 2 + 1);
> +	if (hex == NULL)
> +		return NULL;
> +
> +	for (i = 0, j = 0; i < text_len; i++, j+=2)
> +		g_snprintf(hex + j, 3, "%02x", (unsigned char)text[i]);
> +
> +	hex[j] = 0;
> +
> +	return hex;
> +}
> +

With the above comment in mind, I don't see the need for this at all...

> +static void element_body_parse(GMarkupParseContext *context,
> +				const gchar *text, gsize text_len,
> +				gpointer user_data, GError **error)
> +{
> +	struct provider_info *data = user_data;
> +	struct ofono_gprs_provision_data *ap = data->ap;
> +	gchar *body;
> +
> +	if (data->match_found == FALSE || text_len == 0 ||
> +			data->ap_ignore == TRUE)
> +		return;
> +
> +	body = body_text_parse(text, text_len);
> +	if (body == NULL) {
> +		if (data->element == ELEMENT_SPN) {
> +			char *hex = body_to_hex(text, text_len);
> +			ofono_warn("Can't parse SPN body, len: %lu hex: %s",
> +					text_len, hex);
> +			g_free(hex);
> +			data->match_found = FALSE;
> +		}
> +		return;

You do realize you can set the GError **error argument here and the
parser will bail out properly ;)

> +	}
> +
> +	if (data->element == ELEMENT_SPN) {
> +		data->spn = g_strdup(body);
> +		data->match_found = FALSE;
> +		g_free(body);
> +		return;
> +	}
> +
> +	DBG("%s: '%s'", element_type_name(data->element), body);
> +
> +	switch (data->element) {
> +	case ELEMENT_USERNAME:
> +		ap->username = g_strdup(body);
> +		break;
> +	case ELEMENT_PASSWORD:
> +		ap->password = g_strdup(body);
> +		break;
> +	case ELEMENT_NAME:
> +		ap->name = g_strdup(body);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	g_free(body);
> +}
> +
> +static gboolean ap_type_is_configured(struct provider_info *data,
> +					struct ofono_gprs_provision_data *ap)
> +{
> +	switch (ap->type) {
> +	case OFONO_GPRS_CONTEXT_TYPE_INTERNET:
> +		if (data->ap_type_configured &
> +			AP_TYPE_CONFIGURED_FLAG_INTERNET)
> +			return TRUE;
> +		break;
> +	case OFONO_GPRS_CONTEXT_TYPE_MMS:
> +		if (data->ap_type_configured & AP_TYPE_CONFIGURED_FLAG_MMS)
> +			return TRUE;
> +		break;
> +	case OFONO_GPRS_CONTEXT_TYPE_WAP:
> +		if (data->ap_type_configured & AP_TYPE_CONFIGURED_FLAG_WAP)
> +			return TRUE;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return FALSE;
> +}
> +
> +static gboolean 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 TRUE;
> +	default:
> +		break;
> +	}
> +
> +	return FALSE;
> +}
> +
> +static void ap_type_mark_configured(struct provider_info *data,
> +					struct ofono_gprs_provision_data *ap)
> +{
> +	switch (ap->type) {
> +	case OFONO_GPRS_CONTEXT_TYPE_INTERNET:
> +		data->ap_type_configured |= AP_TYPE_CONFIGURED_FLAG_INTERNET;
> +		break;
> +	case OFONO_GPRS_CONTEXT_TYPE_MMS:
> +		data->ap_type_configured |= AP_TYPE_CONFIGURED_FLAG_MMS;
> +		break;
> +	case OFONO_GPRS_CONTEXT_TYPE_WAP:
> +		data->ap_type_configured |= AP_TYPE_CONFIGURED_FLAG_WAP;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static gboolean ap_ignore(struct provider_info *data,
> +				struct ofono_gprs_provision_data *ap)
> +{
> +	if (data->ap_ignore == TRUE)
> +		return TRUE;
> +
> +	if (ap_type_valid(ap) == FALSE) {
> +		DBG("AP '%s', type isn't supported or present, AP ignored",
> +			ap->apn);
> +		return TRUE;
> +	}
> +
> +	if (ap_type_is_configured(data, ap) == TRUE) {
> +		DBG("AP '%s', multiple settings for type '%s' found, "
> +			"fail provisioning",
> +			ap->apn, ap_type_name(ap->type));
> +		data->provisioning_fail = TRUE;
> +		return TRUE;
> +	}
> +
> +	return FALSE;
> +}
> +
> +static void element_end_parse(GMarkupParseContext *context,
> +				const gchar *element_name,
> +				gpointer user_data, GError **error)
> +{
> +	struct provider_info *data = user_data;
> +	struct ofono_gprs_provision_data *ap = data->ap;
> +	enum element_type element;
> +
> +	if (data->match_found == FALSE)
> +		return;
> +
> +	element = element_type(element_name);
> +
> +	if (data->ap_ignore == FALSE)
> +		DBG("%s: '%s'", element_type_name(element), element_name);
> +
> +	switch (element) {
> +	case ELEMENT_APN:
> +		if (ap_ignore(data, ap)) {
> +			ap_delete(data);
> +			if (data->provisioning_fail == TRUE)
> +				data->match_found = FALSE;
> +		} else
> +			ap_type_mark_configured(data, ap);
> +
> +		data->ap_ignore = FALSE;
> +		break;
> +	case ELEMENT_GSM:
> +		data->match_found = FALSE;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +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 = {
> +	element_start_parse,
> +	element_end_parse,
> +	element_body_parse,
> +	NULL,
> +	parser_error,
> +};
> +
> +static void parse_database(const char *data, ssize_t size,
> +	struct provider_info *provider_info)
> +{
> +	GMarkupParseContext *context;
> +	gboolean result;
> +
> +	provider_info->match_found = FALSE;
> +
> +	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 void provider_info_aps_free(struct provider_info *data)
> +{
> +	while (data->ap_count)
> +		ap_delete(data);
> +}
> +
> +static void provider_info_free(struct provider_info *data)
> +{
> +	g_free(data->spn);
> +
> +	data->spn = NULL;
> +}
> +
> +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->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.provisioning_fail == TRUE) {
> +		provider_info_aps_free(&provider_info);
> +		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_aps_free(&provider_info);
> +		provider_info_free(&provider_info);
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(aps, provider_info.settings,
> +		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);

You call provider_info_aps_free and provider_info_free in the error
conditions above, but only provider_info_free here.  Is there a reason?

> +
> +	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)
> +{
> +	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)

Regards,
-Denis

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

[Denis Kenzior]

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

Hi Oleg,

On 09/06/2011 06:59 AM, Oleg Zhurakivskyy wrote:
> Hello Denis,
> 
> On 08/27/2011 03:12 PM, Denis Kenzior wrote:
>>> +struct provider_info {
> [...]
>>> +    gboolean provisioning_fail;
>>
>> I'm not particularly happy with this approach, can't we simply bail out
>> when we detect this condition?  Perhaps we should just invent our own
>> GError and report it when this condition occurs in e.g. start_element /
>> end_element functions.
> 
> Would GError itself just serve the purpose here?
> 
> struct provider_info {
> [...]
>     GError *err;
> };

Nope, not really.  The point is to bail out as soon as possible.  You
can also extend the error to intelligently report such conditions as
mcc/mnc not matching, duplicate detection, etc.

> 
>>> +static enum element_type element_type(const gchar *element)
> [...]
>> I like this optimization, but have you considered using
>> g_markup_parse_context_push / pop to simplify the logic even more?
> 
> In principal, this could potentially eliminate the need to re-parse the
> tag name to enum at the element_end handler, but I am not 100% sure
> before doing it actually.
> 
> On the contrary, using g_markup_parse_context_push / pop might do just
> exactly the opposite, i.e. to complicate the logic, exploding the amount
> of code. Will the error handling become more straightforward and obvious?
> 

Simpler code is not necessarily more compact, quite often it is more
verbose.  That is OK, the preference is always given to code forms that
are easier to follow, even if at the expense of 20-30% extra lines of code.

And yes, my impression is that it will make error handling way easier to
understand.

> Yet, providing we adopt the approach with GError to bail out, the
> re-parsing of the tag name at the element_end happens only within
> mcc/mnc match, so a few access points only.
> 
> Taking everything into account, is this really worth the time and the
> effort?
> 

oFono philosophy is: 'If you're going to do something, then do the best
you can'

>>> +static void element_network_id_parse(struct provider_info *data,
>>> +                    const gchar **attribute_names,
>>> +                    const gchar **attribute_values)
>>> +{
>>> +    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) {
>>> +        if (data->match_spn == NULL) {
>>> +            data->match_found = TRUE;
>>> +            return;
>>> +        }
>>> +
>>> +        if (g_strcmp0(data->spn, data->match_spn) == 0)
>>> +            data->match_found = TRUE;
>>> +    }
>>
>> I just double checked serviceproviders.2.dtd and can't even find an
>> entry for the SPN.  I mentioned this before, but you can't treat the
>> provider name the same as the SPN.  The xml database is simply not setup
>> this way and the results you will get will be wrong.  Can we please drop
>> this check for now?
> 
> Of course, this check can be dropped for now. In the database, there's
> no entry for SPN, only provider name.
> 
> Why the provider name can't be used as SPN? If not using the provider
> name as SPN, how do you imagine this should work?

The mobile-broadband-provider-info database would probably need to be
extended with a fully-fledged SPN element to store this information.
The current database is simply not setup this way.  To give you an
example, in Australia most providers have the SPN structured something
like this:
'<ProviderName> AU'

So it is quite obvious why your logic above will fail ;)

Other times the SPN name is a shortened / stylized name of the provider,
e.g. Virgin Mobile -> Virgin

The current <name> tag under the <provider> tag is really for the user's
benefit.

> 
>>> +static gchar *body_text_parse(const gchar *text, gsize text_len)
>>> +{
>>> +    gchar *body = g_strndup(text, text_len);
>>> +    gchar *print = NULL;
>>> +    int i;
>>> +
>>> +    for (i = 0; body&&  body[i]; i++) {
>>> +        if (g_ascii_isprint(body[i])) {
>>> +            print = g_strescape(body, NULL);
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    g_free(body);
>>> +
>>> +    return print;
>>
>> What in the world is this doing?  Since the body can contain UTF8
>> characters, I don't think you should be escaping anything here.  If
>> you're trying to validate utf8, then you might need to do something else.
>>> +static char *body_to_hex(const gchar *text, gsize text_len)
> [...]
>> With the above comment in mind, I don't see the need for this at all...
> 
> For some tags, the parser calls body multiple times, first few times
> supplying just '\n' and '\t's. Also, some tags have such a body that
> DBG() has trouble to print without escaping. So, what's the method to
> protect against the latter?
> 

This might be easier if you only run the parser on the bodies of the
tags you really care about.  e.g. this problem might just go away when
you use g_markup_parse_context_push/pop.

>> You do realize you can set the GError **error argument here and the
>> parser will bail out properly ;)
> 
> Yes, that should save some CPU cycles, thanks for the idea.
> 
>> You call provider_info_aps_free and provider_info_free in the error
>> conditions above, but only provider_info_free here.  Is there a reason?
> 
> In case the provisioning was successful, the settings are passed to
> oFono and, later, freed in __ofono_gprs_provision_free_settings(), so
> the settings entries themselves must not be freed.
> 

Ok, fair enough.  However, this is non-obvious, so you might want to add
a comment to that effect.

> However, they shouldn't be freed in case the provisioning was requested
> multiple times, and the previous provisioning run was successful, that
> should be corrected.
> 

Regards,
-Denis

[PATCHv5 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 |  695 ++++++++++++++++++++++++++++++
 3 files changed, 715 insertions(+), 6 deletions(-)
 create mode 100644 plugins/mobile-broadband-provider-info.c

-- 
1.7.4.1

[PATCHv5 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 6f5d921..7df0fd3 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -358,6 +358,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
@@ -365,8 +370,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

[PATCHv5 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 a6b4094..966237c 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

[PATCHv5 3/3] Mobile broadband provider info plugin

[Oleg Zhurakivskyy]

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

---
 plugins/mobile-broadband-provider-info.c |  695 ++++++++++++++++++++++++++++++
 1 files changed, 695 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..f8c898e
--- /dev/null
+++ b/plugins/mobile-broadband-provider-info.c
@@ -0,0 +1,695 @@
+/*
+ *
+ *  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 ap_type_configured_flags {
+	AP_TYPE_CONFIGURED_FLAG_INTERNET	= 0x1,
+	AP_TYPE_CONFIGURED_FLAG_MMS		= 0x2,
+	AP_TYPE_CONFIGURED_FLAG_WAP		= 0x4,
+};
+
+enum element_type {
+	ELEMENT_PROVIDER,
+	ELEMENT_SPN,
+	ELEMENT_NETWORK_ID,
+	ELEMENT_GSM,
+	ELEMENT_APN,
+	ELEMENT_USAGE,
+	ELEMENT_PLAN,
+	ELEMENT_NAME,
+	ELEMENT_USERNAME,
+	ELEMENT_PASSWORD,
+	ELEMENT_NONE,
+};
+
+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;
+	gboolean match_found;
+	gboolean ap_ignore;
+	gboolean provisioning_fail;
+	guint8 ap_type_configured;
+};
+
+static struct provider_info provider_info;
+
+static const char *element_type_name(enum element_type element)
+{
+	switch (element) {
+		_(ELEMENT_PROVIDER);
+		_(ELEMENT_SPN);
+		_(ELEMENT_NETWORK_ID);
+		_(ELEMENT_GSM);
+		_(ELEMENT_APN);
+		_(ELEMENT_USAGE);
+		_(ELEMENT_PLAN);
+		_(ELEMENT_NAME);
+		_(ELEMENT_USERNAME);
+		_(ELEMENT_PASSWORD);
+		_(ELEMENT_NONE);
+	}
+
+	return "ELEMENT_<UNKNOWN>";
+}
+
+static const char *ap_type_name(enum ofono_gprs_context_type ap_type)
+{
+	switch (ap_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)
+{
+	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)
+			return ELEMENT_NAME;
+		if (g_str_equal(element, "network-id") == TRUE)
+			return ELEMENT_NETWORK_ID;
+		break;
+	case 'p':
+		if (g_str_equal(element, "provider") == TRUE)
+			return ELEMENT_PROVIDER;
+		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 void element_network_id_parse(struct provider_info *data,
+					const gchar **attribute_names,
+					const gchar **attribute_values)
+{
+	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) {
+		if (data->match_spn == NULL) {
+			data->match_found = TRUE;
+			return;
+		}
+
+		if (g_strcmp0(data->spn, data->match_spn) == 0)
+			data->match_found = TRUE;
+	}
+}
+
+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++;
+
+		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 element_apn_parse(struct provider_info *data,
+				const gchar **attribute_names,
+				const gchar **attribute_values)
+{
+	int i;
+
+	data->ap = ap_new(data);
+	if (data->ap == NULL) {
+		DBG("MAX_APS %d reached, end parsing", MAX_APS);
+		data->match_found = FALSE;
+		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 element_usage_parse(struct provider_info *data,
+				const gchar **attribute_names,
+				const gchar **attribute_values)
+{
+	struct ofono_gprs_provision_data *ap = data->ap;
+	const char *usage = NULL;
+	int i;
+
+	for (i = 0; attribute_names[i]; i++) {
+		if (g_str_equal(attribute_names[i], "type") == TRUE)
+			usage = attribute_values[i];
+	}
+
+	if (usage == NULL) {
+		DBG("AP '%s', no type, AP ignored", ap->apn);
+		data->ap_ignore = TRUE;
+		return;
+	}
+
+	if (g_str_equal(usage, "internet") == TRUE)
+		ap->type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
+	else if (g_str_equal(usage, "mms") == TRUE)
+		ap->type = OFONO_GPRS_CONTEXT_TYPE_MMS;
+	else if (g_str_equal(usage, "wap") == TRUE)
+		ap->type = OFONO_GPRS_CONTEXT_TYPE_WAP;
+}
+
+static void element_plan_parse(struct provider_info *data,
+				const gchar **attribute_names,
+				const gchar **attribute_values)
+{
+	int i;
+
+	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)
+				return;
+
+			DBG("AP '%s', plan '%s' is not supported, AP ignored",
+				data->ap->apn, attribute_values[i]);
+			break;
+		}
+
+	data->ap_ignore = TRUE;
+}
+
+static void element_start_parse(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 prev = data->element;
+
+	data->element = element_type(element_name);
+
+	if (prev == ELEMENT_PROVIDER && data->element == ELEMENT_NAME) {
+		data->element = ELEMENT_SPN;
+		g_free(data->spn);
+		data->spn = NULL;
+		data->match_found = TRUE;
+		return;
+	}
+
+	if (data->element == ELEMENT_NETWORK_ID)
+		element_network_id_parse(data, attribute_names,
+						attribute_values);
+
+	if (data->match_found == FALSE || data->ap_ignore == TRUE)
+		return;
+
+	DBG("%s: '%s'", element_type_name(data->element), element_name);
+
+	switch (data->element) {
+	case ELEMENT_APN:
+		element_apn_parse(data, attribute_names, attribute_values);
+		break;
+	case ELEMENT_USAGE:
+		element_usage_parse(data, attribute_names, attribute_values);
+		break;
+	case ELEMENT_PLAN:
+		element_plan_parse(data, attribute_names, attribute_values);
+		break;
+	default:
+		break;
+	}
+}
+
+static gchar *body_text_parse(const gchar *text, gsize text_len)
+{
+	gchar *body = g_strndup(text, text_len);
+	gchar *print = NULL;
+	int i;
+
+	for (i = 0; body && body[i]; i++) {
+		if (g_ascii_isprint(body[i])) {
+			print = g_strescape(body, NULL);
+			break;
+		}
+	}
+
+	g_free(body);
+
+	return print;
+}
+
+static char *body_to_hex(const gchar *text, gsize text_len)
+{
+	char *hex;
+	guint i, j;
+
+	hex = g_try_malloc(text_len * 2 + 1);
+	if (hex == NULL)
+		return NULL;
+
+	for (i = 0, j = 0; i < text_len; i++, j+=2)
+		g_snprintf(hex + j, 3, "%02x", (unsigned char)text[i]);
+
+	hex[j] = 0;
+
+	return hex;
+}
+
+static void element_body_parse(GMarkupParseContext *context,
+				const gchar *text, gsize text_len,
+				gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+	struct ofono_gprs_provision_data *ap = data->ap;
+	gchar *body;
+
+	if (data->match_found == FALSE || text_len == 0 ||
+			data->ap_ignore == TRUE)
+		return;
+
+	body = body_text_parse(text, text_len);
+	if (body == NULL) {
+		if (data->element == ELEMENT_SPN) {
+			char *hex = body_to_hex(text, text_len);
+			ofono_warn("Can't parse SPN body, len: %lu hex: %s",
+					text_len, hex);
+			g_free(hex);
+			data->match_found = FALSE;
+		}
+		return;
+	}
+
+	if (data->element == ELEMENT_SPN) {
+		data->spn = g_strdup(body);
+		data->match_found = FALSE;
+		g_free(body);
+		return;
+	}
+
+	DBG("%s: '%s'", element_type_name(data->element), body);
+
+	switch (data->element) {
+	case ELEMENT_USERNAME:
+		ap->username = g_strdup(body);
+		break;
+	case ELEMENT_PASSWORD:
+		ap->password = g_strdup(body);
+		break;
+	case ELEMENT_NAME:
+		ap->name = g_strdup(body);
+		break;
+	default:
+		break;
+	}
+
+	g_free(body);
+}
+
+static gboolean ap_type_is_configured(struct provider_info *data,
+					struct ofono_gprs_provision_data *ap)
+{
+	switch (ap->type) {
+	case OFONO_GPRS_CONTEXT_TYPE_INTERNET:
+		if (data->ap_type_configured &
+			AP_TYPE_CONFIGURED_FLAG_INTERNET)
+			return TRUE;
+		break;
+	case OFONO_GPRS_CONTEXT_TYPE_MMS:
+		if (data->ap_type_configured & AP_TYPE_CONFIGURED_FLAG_MMS)
+			return TRUE;
+		break;
+	case OFONO_GPRS_CONTEXT_TYPE_WAP:
+		if (data->ap_type_configured & AP_TYPE_CONFIGURED_FLAG_WAP)
+			return TRUE;
+		break;
+	default:
+		break;
+	}
+
+	return FALSE;
+}
+
+static gboolean 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 TRUE;
+	default:
+		break;
+	}
+
+	return FALSE;
+}
+
+static void ap_type_mark_configured(struct provider_info *data,
+					struct ofono_gprs_provision_data *ap)
+{
+	switch (ap->type) {
+	case OFONO_GPRS_CONTEXT_TYPE_INTERNET:
+		data->ap_type_configured |= AP_TYPE_CONFIGURED_FLAG_INTERNET;
+		break;
+	case OFONO_GPRS_CONTEXT_TYPE_MMS:
+		data->ap_type_configured |= AP_TYPE_CONFIGURED_FLAG_MMS;
+		break;
+	case OFONO_GPRS_CONTEXT_TYPE_WAP:
+		data->ap_type_configured |= AP_TYPE_CONFIGURED_FLAG_WAP;
+		break;
+	default:
+		break;
+	}
+}
+
+static gboolean ap_ignore(struct provider_info *data,
+				struct ofono_gprs_provision_data *ap)
+{
+	if (data->ap_ignore == TRUE)
+		return TRUE;
+
+	if (ap_type_valid(ap) == FALSE) {
+		DBG("AP '%s', type isn't supported or present, AP ignored",
+			ap->apn);
+		return TRUE;
+	}
+
+	if (ap_type_is_configured(data, ap) == TRUE) {
+		DBG("AP '%s', multiple settings for type '%s' found, "
+			"fail provisioning",
+			ap->apn, ap_type_name(ap->type));
+		data->provisioning_fail = TRUE;
+		return TRUE;
+	}
+
+	return FALSE;
+}
+
+static void element_end_parse(GMarkupParseContext *context,
+				const gchar *element_name,
+				gpointer user_data, GError **error)
+{
+	struct provider_info *data = user_data;
+	struct ofono_gprs_provision_data *ap = data->ap;
+	enum element_type element;
+
+	if (data->match_found == FALSE)
+		return;
+
+	element = element_type(element_name);
+
+	if (data->ap_ignore == FALSE)
+		DBG("%s: '%s'", element_type_name(element), element_name);
+
+	switch (element) {
+	case ELEMENT_APN:
+		if (ap_ignore(data, ap)) {
+			ap_delete(data);
+			if (data->provisioning_fail == TRUE)
+				data->match_found = FALSE;
+		} else
+			ap_type_mark_configured(data, ap);
+
+		data->ap_ignore = FALSE;
+		break;
+	case ELEMENT_GSM:
+		data->match_found = FALSE;
+		break;
+	default:
+		break;
+	}
+}
+
+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 = {
+	element_start_parse,
+	element_end_parse,
+	element_body_parse,
+	NULL,
+	parser_error,
+};
+
+static void parse_database(const char *data, ssize_t size,
+	struct provider_info *provider_info)
+{
+	GMarkupParseContext *context;
+	gboolean result;
+
+	provider_info->match_found = FALSE;
+
+	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 void provider_info_aps_free(struct provider_info *data)
+{
+	while (data->ap_count)
+		ap_delete(data);
+}
+
+static void provider_info_free(struct provider_info *data)
+{
+	g_free(data->spn);
+
+	data->spn = NULL;
+}
+
+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->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.provisioning_fail == TRUE) {
+		provider_info_aps_free(&provider_info);
+		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_aps_free(&provider_info);
+		provider_info_free(&provider_info);
+		return -ENOMEM;
+	}
+
+	memcpy(aps, provider_info.settings,
+		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)
+{
+	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: [PATCHv5 3/3] Mobile broadband provider info plugin

[Denis Kenzior]

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

Hi Oleg,

On 09/07/2011 07:38 AM, Oleg Zhurakivskyy wrote:
> Hello Denis,
> 
> On 08/30/2011 11:17 AM, Denis Kenzior wrote:
>> The mobile-broadband-provider-info database would probably need to be
>> extended with a fully-fledged SPN element to store this information.
>> The current database is simply not setup this way.  To give you an
>> example, in Australia most providers have the SPN structured something
>> like this:
>> '<ProviderName>  AU'
>>
>> So it is quite obvious why your logic above will fail ;)
>>
>> Other times the SPN name is a shortened / stylized name of the provider,
>> e.g. Virgin Mobile ->  Virgin
>>
>> The current<name>  tag under the<provider>  tag is really for the user's
>> benefit.
> 
> If the provider name isn't sufficient to serve as the SPN, shall we add
> optional SPN element on the same level?
> 
> <provider>
>     <name>...</name>
>     <spn>...</spn>
>     <network-id mcc="123" mnc="12"/>
>     <apn>...</apn>
> </provider>
> 
> Can we drop this check until such SPN element can be upstreamed into the
> database?
> 

Yes, this is exactly what I'd like to do.

>>> For some tags, the parser calls body multiple times, first few times
>>> supplying just '\n' and '\t's. Also, some tags have such a body that
>>> DBG() has trouble to print without escaping. So, what's the method to
>>> protect against the latter?
>>
>> This might be easier if you only run the parser on the bodies of the
>> tags you really care about.  e.g. this problem might just go away when
>> you use g_markup_parse_context_push/pop.
> 
> This place in the parser is already run only on bodies we care about,
> i.e. within mcc/mnc match. If the diagnostics on body is required, this
> problem won't go away if we use g_markup_parse_context_push/pop. So, do
> we keep the escaping or drop diagnostics of the element body? Or,
> propose a solution?

I still think that the reasons for your escaping logic are flawed.  I
suggest that you study the problem and get a better understanding why
this happens in the first place.

For example:
<network-id mcc="412" mnc="01"/>
<apn value="internet">
	<username>awcc</username>
	<password>1111</password>
</apn>

I expect the body callback to be called with random \r\t\n garbage for
the APN tag.  However, this is not something we care about anyway and
can ignore (e.g. with a call to g_markup_parse_context_push)

For username/password bodies we shouldn't even have \r\t\n characters in
there in the first place.  Certainly the naive escaping mechanism you
have now won't work with UTF-8 strings.

Regards,
-Denis

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

[Oleg Zhurakivskyy]

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

Hello Denis,

On 08/27/2011 03:12 PM, Denis Kenzior wrote:
>> +struct provider_info {
[...]
>> +	gboolean provisioning_fail;
>
> I'm not particularly happy with this approach, can't we simply bail out
> when we detect this condition?  Perhaps we should just invent our own
> GError and report it when this condition occurs in e.g. start_element /
> end_element functions.

Would GError itself just serve the purpose here?

struct provider_info {
[...]
	GError *err;
};

>> +static enum element_type element_type(const gchar *element)
[...]
> I like this optimization, but have you considered using
> g_markup_parse_context_push / pop to simplify the logic even more?

In principal, this could potentially eliminate the need to re-parse the tag name 
to enum at the element_end handler, but I am not 100% sure before doing it actually.

On the contrary, using g_markup_parse_context_push / pop might do just exactly 
the opposite, i.e. to complicate the logic, exploding the amount of code. Will 
the error handling become more straightforward and obvious?

Yet, providing we adopt the approach with GError to bail out, the re-parsing of 
the tag name at the element_end happens only within mcc/mnc match, so a few 
access points only.

Taking everything into account, is this really worth the time and the effort?

>> +static void element_network_id_parse(struct provider_info *data,
>> +					const gchar **attribute_names,
>> +					const gchar **attribute_values)
>> +{
>> +	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) {
>> +		if (data->match_spn == NULL) {
>> +			data->match_found = TRUE;
>> +			return;
>> +		}
>> +
>> +		if (g_strcmp0(data->spn, data->match_spn) == 0)
>> +			data->match_found = TRUE;
>> +	}
>
> I just double checked serviceproviders.2.dtd and can't even find an
> entry for the SPN.  I mentioned this before, but you can't treat the
> provider name the same as the SPN.  The xml database is simply not setup
> this way and the results you will get will be wrong.  Can we please drop
> this check for now?

Of course, this check can be dropped for now. In the database, there's no entry 
for SPN, only provider name.

Why the provider name can't be used as SPN? If not using the provider name as 
SPN, how do you imagine this should work?

>> +static gchar *body_text_parse(const gchar *text, gsize text_len)
>> +{
>> +	gchar *body = g_strndup(text, text_len);
>> +	gchar *print = NULL;
>> +	int i;
>> +
>> +	for (i = 0; body&&  body[i]; i++) {
>> +		if (g_ascii_isprint(body[i])) {
>> +			print = g_strescape(body, NULL);
>> +			break;
>> +		}
>> +	}
>> +
>> +	g_free(body);
>> +
>> +	return print;
>
> What in the world is this doing?  Since the body can contain UTF8
> characters, I don't think you should be escaping anything here.  If
> you're trying to validate utf8, then you might need to do something else.
>> +static char *body_to_hex(const gchar *text, gsize text_len)
[...]
> With the above comment in mind, I don't see the need for this at all...

For some tags, the parser calls body multiple times, first few times supplying 
just '\n' and '\t's. Also, some tags have such a body that DBG() has trouble to 
print without escaping. So, what's the method to protect against the latter?

> You do realize you can set the GError **error argument here and the
> parser will bail out properly ;)

Yes, that should save some CPU cycles, thanks for the idea.

> You call provider_info_aps_free and provider_info_free in the error
> conditions above, but only provider_info_free here.  Is there a reason?

In case the provisioning was successful, the settings are passed to oFono and, 
later, freed in __ofono_gprs_provision_free_settings(), so the settings entries 
themselves must not be freed.

However, they shouldn't be freed in case the provisioning was requested multiple 
times, and the previous provisioning run was successful, that should be corrected.

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

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

[Oleg Zhurakivskyy]

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

Hello Denis,

On 08/30/2011 11:17 AM, Denis Kenzior wrote:
> The mobile-broadband-provider-info database would probably need to be
> extended with a fully-fledged SPN element to store this information.
> The current database is simply not setup this way.  To give you an
> example, in Australia most providers have the SPN structured something
> like this:
> '<ProviderName>  AU'
>
> So it is quite obvious why your logic above will fail ;)
>
> Other times the SPN name is a shortened / stylized name of the provider,
> e.g. Virgin Mobile ->  Virgin
>
> The current<name>  tag under the<provider>  tag is really for the user's
> benefit.

If the provider name isn't sufficient to serve as the SPN, shall we add optional 
SPN element on the same level?

<provider>
	<name>...</name>
	<spn>...</spn>
	<network-id mcc="123" mnc="12"/>
	<apn>...</apn>
</provider>

Can we drop this check until such SPN element can be upstreamed into the database?

>> For some tags, the parser calls body multiple times, first few times
>> supplying just '\n' and '\t's. Also, some tags have such a body that
>> DBG() has trouble to print without escaping. So, what's the method to
>> protect against the latter?
>
> This might be easier if you only run the parser on the bodies of the
> tags you really care about.  e.g. this problem might just go away when
> you use g_markup_parse_context_push/pop.

This place in the parser is already run only on bodies we care about, i.e. 
within mcc/mnc match. If the diagnostics on body is required, this problem won't 
go away if we use g_markup_parse_context_push/pop. So, do we keep the escaping 
or drop diagnostics of the element body? Or, propose a solution?

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