[PATCH] mbpi: Parse gsm provider name

[PATCH] mbpi: Parse gsm provider name

[Slava Monich]

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

This gives the provisioning plugin more information to work with and
improves the chance of automatically picking the right AP in case if
we have more than one for the same mcc/mnc combination.
---
 include/gprs-provision.h |  1 +
 plugins/mbpi.c           | 47 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/include/gprs-provision.h b/include/gprs-provision.h
index e9eec61..0129cd0 100644
--- a/include/gprs-provision.h
+++ b/include/gprs-provision.h
@@ -31,6 +31,7 @@ extern "C" {
 struct ofono_gprs_provision_data {
 	enum ofono_gprs_context_type type;
 	enum ofono_gprs_proto proto;
+	char *provider_name;
 	char *name;
 	char *apn;
 	char *username;
diff --git a/plugins/mbpi.c b/plugins/mbpi.c
index dff8752..f2b00d0 100644
--- a/plugins/mbpi.c
+++ b/plugins/mbpi.c
@@ -53,6 +53,7 @@ enum MBPI_ERROR {
 struct gsm_data {
 	const char *match_mcc;
 	const char *match_mnc;
+	char *provider_name;
 	GSList *apns;
 	gboolean match_found;
 	gboolean allow_duplicates;
@@ -84,6 +85,7 @@ static GQuark mbpi_error_quark(void)
 
 void mbpi_ap_free(struct ofono_gprs_provision_data *ap)
 {
+	g_free(ap->provider_name);
 	g_free(ap->name);
 	g_free(ap->apn);
 	g_free(ap->username);
@@ -117,6 +119,7 @@ static void text_handler(GMarkupParseContext *context,
 {
 	char **string = userdata;
 
+        g_free(*string);
 	*string = g_strndup(text, text_len);
 }
 
@@ -288,6 +291,7 @@ static void apn_handler(GMarkupParseContext *context, struct gsm_data *gsm,
 	}
 
 	ap = g_new0(struct ofono_gprs_provision_data, 1);
+	ap->provider_name = g_strdup(gsm->provider_name);
 	ap->apn = g_strdup(apn);
 	ap->type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
 	ap->proto = OFONO_GPRS_PROTO_IP;
@@ -454,7 +458,7 @@ static const GMarkupParser provider_parser = {
 	NULL,
 };
 
-static void toplevel_gsm_start(GMarkupParseContext *context,
+static void gsm_provider_start(GMarkupParseContext *context,
 					const gchar *element_name,
 					const gchar **atribute_names,
 					const gchar **attribute_values,
@@ -462,19 +466,53 @@ static void toplevel_gsm_start(GMarkupParseContext *context,
 {
 	struct gsm_data *gsm = userdata;
 
-	if (g_str_equal(element_name, "gsm")) {
+	if (g_str_equal(element_name, "name")) {
+		g_free(gsm->provider_name);
+		gsm->provider_name = NULL;
+		g_markup_parse_context_push(context, &text_parser,
+						&gsm->provider_name);
+	} else if (g_str_equal(element_name, "gsm")) {
 		gsm->match_found = FALSE;
 		g_markup_parse_context_push(context, &gsm_parser, gsm);
 	} else if (g_str_equal(element_name, "cdma"))
 		g_markup_parse_context_push(context, &skip_parser, NULL);
 }
 
+static void gsm_provider_end(GMarkupParseContext *context,
+					const gchar *element_name,
+					gpointer userdata, GError **error)
+{
+	if (g_str_equal(element_name, "name") ||
+				g_str_equal(element_name, "gsm") ||
+				g_str_equal(element_name, "cdma"))
+		g_markup_parse_context_pop(context);
+}
+
+static const GMarkupParser gsm_provider_parser = {
+	gsm_provider_start,
+	gsm_provider_end,
+	NULL,
+	NULL,
+	NULL,
+};
+
+static void toplevel_gsm_start(GMarkupParseContext *context,
+					const gchar *element_name,
+					const gchar **atribute_names,
+					const gchar **attribute_values,
+					gpointer userdata, GError **error)
+{
+	struct gsm_data *gsm = userdata;
+
+	if (g_str_equal(element_name, "provider"))
+		g_markup_parse_context_push(context, &gsm_provider_parser, gsm);
+}
+
 static void toplevel_gsm_end(GMarkupParseContext *context,
 					const gchar *element_name,
 					gpointer userdata, GError **error)
 {
-	if (g_str_equal(element_name, "gsm") ||
-			g_str_equal(element_name, "cdma"))
+	if (g_str_equal(element_name, "provider"))
 		g_markup_parse_context_pop(context);
 }
 
@@ -591,6 +629,7 @@ GSList *mbpi_lookup_apn(const char *mcc, const char *mnc,
 		gsm.apns = NULL;
 	}
 
+	g_free(gsm.provider_name);
 	return gsm.apns;
 }
 
-- 
1.8.3.2

Re: [PATCH] mbpi: Parse gsm provider name

[Denis Kenzior]

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

Hi Slava,

On 04/16/2014 09:00 AM, Slava Monich wrote:
> This gives the provisioning plugin more information to work with and
> improves the chance of automatically picking the right AP in case if
> we have more than one for the same mcc/mnc combination.
> ---
>  include/gprs-provision.h |  1 +
>  plugins/mbpi.c           | 47 +++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 44 insertions(+), 4 deletions(-)
> 

I fail to see the point for this patch.  It makes no sense on its own.

> diff --git a/include/gprs-provision.h b/include/gprs-provision.h
> index e9eec61..0129cd0 100644
> --- a/include/gprs-provision.h
> +++ b/include/gprs-provision.h
> @@ -31,6 +31,7 @@ extern "C" {
>  struct ofono_gprs_provision_data {
>  	enum ofono_gprs_context_type type;
>  	enum ofono_gprs_proto proto;
> +	char *provider_name;

I fail to see why this would be required here?  It is not utilized
anywhere else...

Regards,
-Denis

Re: [PATCH] mbpi: Parse gsm provider name

[Slava Monich]

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

Hi Denis,
It's for custom provisioning plugins.

Service provider name is passed to the provisioning plugin but not used 
there at all. This patch makes it possible to utilize it for AP matching.

Regards,
-Slava


On 17/04/14 07:07, Denis Kenzior wrote:
> Hi Slava,
>
> On 04/16/2014 09:00 AM, Slava Monich wrote:
>> This gives the provisioning plugin more information to work with and
>> improves the chance of automatically picking the right AP in case if
>> we have more than one for the same mcc/mnc combination.
>> ---
>>   include/gprs-provision.h |  1 +
>>   plugins/mbpi.c           | 47 +++++++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 44 insertions(+), 4 deletions(-)
>>
> I fail to see the point for this patch.  It makes no sense on its own.
>
>> diff --git a/include/gprs-provision.h b/include/gprs-provision.h
>> index e9eec61..0129cd0 100644
>> --- a/include/gprs-provision.h
>> +++ b/include/gprs-provision.h
>> @@ -31,6 +31,7 @@ extern "C" {
>>   struct ofono_gprs_provision_data {
>>   	enum ofono_gprs_context_type type;
>>   	enum ofono_gprs_proto proto;
>> +	char *provider_name;
> I fail to see why this would be required here?  It is not utilized
> anywhere else...
>
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
>

Re: [PATCH] mbpi: Parse gsm provider name

[Denis Kenzior]

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

Hi Slava,

Please do not top-post on this mailing list.

On 04/17/2014 03:24 AM, Slava Monich wrote:
> Hi Denis,
> It's for custom provisioning plugins.
> 
> Service provider name is passed to the provisioning plugin but not used
> there at all. This patch makes it possible to utilize it for AP matching.
> 

I still do not understand.  SPN is passed to the provisioning plugin via
get_settings.  I do not see any need for it to be stored in
ofono_gprs_provision_data struct.  How is it going to be used by the core?

Regards,
-Denis

Re: [PATCH] mbpi: Parse gsm provider name

[Slava Monich]

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

Hi Denis,

> Hi Slava,
>
> Please do not top-post on this mailing list.

ah, sorry.

>
> On 04/17/2014 03:24 AM, Slava Monich wrote:
>> Hi Denis,
>> It's for custom provisioning plugins.
>>
>> Service provider name is passed to the provisioning plugin but not used
>> there at all. This patch makes it possible to utilize it for AP matching.
>>
> I still do not understand.  SPN is passed to the provisioning plugin via
> get_settings.  I do not see any need for it to be stored in
> ofono_gprs_provision_data struct.  How is it going to be used by the core?

I agree that there is no need to for it to be returned by the 
provisioning plugin to the core but it's the same structure used by 
mbpi.c to return the result of parsing serviceproviders.xml to the 
provisioning plugin. Side effect of sharing the structure between two 
interfaces.

If instead I changed the interface between mbpi and provisioning plugin 
and kept the interface with the core intact, would that look acceptable 
to you?

Thanks,
-Slava

Re: [PATCH] mbpi: Parse gsm provider name

[Denis Kenzior]

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

Hi Slava,

> I agree that there is no need to for it to be returned by the
> provisioning plugin to the core but it's the same structure used by
> mbpi.c to return the result of parsing serviceproviders.xml to the
> provisioning plugin. Side effect of sharing the structure between two
> interfaces.

'Side-effects' that affect the core are never acceptable.  Just a
friendly hint that any changes to the core automatically trigger
scrutiny beyond our already thorough review process.

> If instead I changed the interface between mbpi and provisioning plugin
> and kept the interface with the core intact, would that look acceptable
> to you?

Perhaps, but I'm still failing to see why the service provider name as
entered in MBPI is useful to anyone.

Regards,
-Denis

Re: [PATCH] mbpi: Parse gsm provider name

[Slava Monich]

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

Hi Denis,
>> If instead I changed the interface between mbpi and provisioning plugin
>> and kept the interface with the core intact, would that look acceptable
>> to you?
> Perhaps, but I'm still failing to see why the service provider name as
> entered in MBPI is useful to anyone.

Operators share mcc/mnc meaning that automatically finding the best AP 
(the one most likely to work) from the database may involve some sort of 
fuzzy logic (partial or case-insensitive name comparison or something 
like this) implemented in a product-specific provisioning plugin. Making 
things work out-the-box translates into fewer customer complaints, 
better user experience and is generally the right thing to do.

I was trying to share a piece of code that doesn't involve any 
product-specific logic and yet might be useful to others.

Please let me know if I should give it another try, otherwise I won't 
waste anyone's time and this code will just remain in our branch of ofono.

Thanks,
-Slava

Re: [PATCH] mbpi: Parse gsm provider name

[Denis Kenzior]

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

Hi Slava,

> Operators share mcc/mnc meaning that automatically finding the best AP
> (the one most likely to work) from the database may involve some sort of
> fuzzy logic (partial or case-insensitive name comparison or something
> like this) implemented in a product-specific provisioning plugin. Making
> things work out-the-box translates into fewer customer complaints,
> better user experience and is generally the right thing to do.
> 
> I was trying to share a piece of code that doesn't involve any
> product-specific logic and yet might be useful to others.
> 
> Please let me know if I should give it another try, otherwise I won't
> waste anyone's time and this code will just remain in our branch of ofono.

So you're trying to parse and return provider name information from mbpi
and return it back to the caller.  Then the caller will try to
'fuzzy-logic' and match it against the SPN.  Right?  Why not make things
easy on yourself and fix the database for the operators you need?  But I
digress.

Feel free to try.  If you can do this cleanly, then I have no problem
with it in principle.

Regards,
-Denis

Re: [PATCH] mbpi: Parse gsm provider name

[Slava Monich]

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

Hi Denis,

> Hi Slava,
>
>> Operators share mcc/mnc meaning that automatically finding the best AP
>> (the one most likely to work) from the database may involve some sort of
>> fuzzy logic (partial or case-insensitive name comparison or something
>> like this) implemented in a product-specific provisioning plugin. Making
>> things work out-the-box translates into fewer customer complaints,
>> better user experience and is generally the right thing to do.
>>
>> I was trying to share a piece of code that doesn't involve any
>> product-specific logic and yet might be useful to others.
>>
>> Please let me know if I should give it another try, otherwise I won't
>> waste anyone's time and this code will just remain in our branch of ofono.
> So you're trying to parse and return provider name information from mbpi
> and return it back to the caller.  Then the caller will try to
> 'fuzzy-logic' and match it against the SPN.  Right?  Why not make things
> easy on yourself and fix the database for the operators you need?  But I
> digress.
>
> Feel free to try.  If you can do this cleanly, then I have no problem
> with it in principle.


Even if the operator database was 100% accurate (which is practically 
impossible) something would still have to be done about the operators 
sharing MCC/MNC and yet using different AP settings.

I'll give it another try.

Thanks,
-Slava

Re: [PATCH] mbpi: Parse gsm provider name

[Denis Kenzior]

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

Hi Slava,

> Even if the operator database was 100% accurate (which is practically
> impossible) something would still have to be done about the operators
> sharing MCC/MNC and yet using different AP settings.

We already have such a selector, it is called Service Provider Name or
SPN.  This is read from the SIM and just about every provider populates
(they want their operator name to be displayed correctly after all).

Regards,
-Denis