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

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Oleg,

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

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

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

> diff --git a/plugins/gprs-provision.c b/plugins/gprs-provision.c
> new file mode 100644
> index 0000000..015ea7e
> --- /dev/null
> +++ b/plugins/gprs-provision.c
> @@ -0,0 +1,290 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <glib.h>
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono/types.h>
> +#include <ofono/log.h>
> +#include <ofono/plugin.h>
> +#include <ofono/modem.h>
> +#include <ofono/gprs-provision.h>
> +
> +#define PROVISION_SETTINGS_MAX 10

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

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

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

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

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

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

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

And such a debug should be before any code execution.

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

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

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

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

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

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

Regards

Marcel