* [PATCHv8 0/6] Provisioning plugin
@ 2011-09-30 13:31 Oleg Zhurakivskyy
2011-09-30 13:31 ` [PATCHv8 1/6] plugins: Provisioning plugin autoconf support Oleg Zhurakivskyy
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Oleg Zhurakivskyy @ 2011-09-30 13:31 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCHv8 1/6] plugins: Provisioning plugin autoconf support 2011-09-30 13:31 [PATCHv8 0/6] Provisioning plugin Oleg Zhurakivskyy @ 2011-09-30 13:31 ` Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 2/6] plugins: Provisioning plugin makefile changes Oleg Zhurakivskyy ` (4 subsequent siblings) 5 siblings, 0 replies; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-09-30 13:31 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv8 2/6] plugins: Provisioning plugin makefile changes 2011-09-30 13:31 [PATCHv8 0/6] Provisioning plugin Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 1/6] plugins: Provisioning plugin autoconf support Oleg Zhurakivskyy @ 2011-09-30 13:31 ` Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes Oleg Zhurakivskyy ` (3 subsequent siblings) 5 siblings, 0 replies; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-09-30 13:31 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes 2011-09-30 13:31 [PATCHv8 0/6] Provisioning plugin Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 1/6] plugins: Provisioning plugin autoconf support Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 2/6] plugins: Provisioning plugin makefile changes Oleg Zhurakivskyy @ 2011-09-30 13:31 ` Oleg Zhurakivskyy 2011-09-30 14:57 ` Denis Kenzior 2011-09-30 13:31 ` [PATCHv8 4/6] " Oleg Zhurakivskyy ` (2 subsequent siblings) 5 siblings, 1 reply; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-09-30 13:31 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes 2011-09-30 13:31 ` [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes Oleg Zhurakivskyy @ 2011-09-30 14:57 ` Denis Kenzior 2011-10-04 9:33 ` Oleg Zhurakivskyy 0 siblings, 1 reply; 13+ messages in thread From: Denis Kenzior @ 2011-09-30 14:57 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes 2011-09-30 14:57 ` Denis Kenzior @ 2011-10-04 9:33 ` Oleg Zhurakivskyy 0 siblings, 0 replies; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-10-04 9:33 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes 2011-09-30 13:31 [PATCHv8 0/6] Provisioning plugin Oleg Zhurakivskyy ` (2 preceding siblings ...) 2011-09-30 13:31 ` [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes Oleg Zhurakivskyy @ 2011-09-30 13:31 ` Oleg Zhurakivskyy 2011-09-30 15:20 ` Denis Kenzior 2011-09-30 13:31 ` [PATCHv8 5/6] plugins: Add provisioning plugin Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 6/6] tools: lookup-apn update Oleg Zhurakivskyy 5 siblings, 1 reply; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-09-30 13:31 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes 2011-09-30 13:31 ` [PATCHv8 4/6] " Oleg Zhurakivskyy @ 2011-09-30 15:20 ` Denis Kenzior 2011-10-04 9:33 ` Oleg Zhurakivskyy 0 siblings, 1 reply; 13+ messages in thread From: Denis Kenzior @ 2011-09-30 15:20 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes 2011-09-30 15:20 ` Denis Kenzior @ 2011-10-04 9:33 ` Oleg Zhurakivskyy 2011-10-04 14:05 ` Denis Kenzior 0 siblings, 1 reply; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-10-04 9:33 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes 2011-10-04 9:33 ` Oleg Zhurakivskyy @ 2011-10-04 14:05 ` Denis Kenzior 2011-10-05 8:07 ` Oleg Zhurakivskyy 0 siblings, 1 reply; 13+ messages in thread From: Denis Kenzior @ 2011-10-04 14:05 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv8 4/6] plugins: mobile-broadband-provider-info parser changes 2011-10-04 14:05 ` Denis Kenzior @ 2011-10-05 8:07 ` Oleg Zhurakivskyy 0 siblings, 0 replies; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-10-05 8:07 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv8 5/6] plugins: Add provisioning plugin 2011-09-30 13:31 [PATCHv8 0/6] Provisioning plugin Oleg Zhurakivskyy ` (3 preceding siblings ...) 2011-09-30 13:31 ` [PATCHv8 4/6] " Oleg Zhurakivskyy @ 2011-09-30 13:31 ` Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 6/6] tools: lookup-apn update Oleg Zhurakivskyy 5 siblings, 0 replies; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-09-30 13:31 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv8 6/6] tools: lookup-apn update 2011-09-30 13:31 [PATCHv8 0/6] Provisioning plugin Oleg Zhurakivskyy ` (4 preceding siblings ...) 2011-09-30 13:31 ` [PATCHv8 5/6] plugins: Add provisioning plugin Oleg Zhurakivskyy @ 2011-09-30 13:31 ` Oleg Zhurakivskyy 5 siblings, 0 replies; 13+ messages in thread From: Oleg Zhurakivskyy @ 2011-09-30 13:31 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-10-05 8:07 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-30 13:31 [PATCHv8 0/6] Provisioning plugin Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 1/6] plugins: Provisioning plugin autoconf support Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 2/6] plugins: Provisioning plugin makefile changes Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 3/6] plugins: mobile-broadband-provider-info parser changes Oleg Zhurakivskyy 2011-09-30 14:57 ` Denis Kenzior 2011-10-04 9:33 ` Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 4/6] " Oleg Zhurakivskyy 2011-09-30 15:20 ` Denis Kenzior 2011-10-04 9:33 ` Oleg Zhurakivskyy 2011-10-04 14:05 ` Denis Kenzior 2011-10-05 8:07 ` Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 5/6] plugins: Add provisioning plugin Oleg Zhurakivskyy 2011-09-30 13:31 ` [PATCHv8 6/6] tools: lookup-apn update Oleg Zhurakivskyy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.