--- 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

Hi Oleg, On 09/30/2011 08:31 AM, Oleg Zhurakivskyy 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. Regards, -Denis

--- 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

Hello Denis, On 09/30/2011 06:20 PM, Denis Kenzior wrote: OK. OK. 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? 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? 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? OK. OK. OK. 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

--- 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