[PATCH] gprs: fix allocation of context id

[PATCH] gprs: fix allocation of context id

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

After the convertion to l_uintset, the creation of new contexts fails
due to a range error being returned from l_uintset_find_unused().

The error happens because the uinset is created with a min-value of 1,
but the start-value passed to l_uintset_find_unused() is initialized as
0.

Fix this by passing a start-value just past the last allocated context
id.
---
 src/gprs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gprs.c b/src/gprs.c
index 7e9e5161..582e3207 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -2329,7 +2329,7 @@ static void provision_context(const struct ofono_gprs_provision_data *ap,
 			strlen(ap->message_center) > MAX_MESSAGE_CENTER_LENGTH)
 		return;
 
-	id = l_uintset_find_unused(gprs->used_pids, gprs->last_context_id);
+	id = l_uintset_find_unused(gprs->used_pids, gprs->last_context_id + 1);
 	if (id > l_uintset_get_max(gprs->used_pids))
 		return;
 
-- 
2.20.1

[PATCH] provision: allow duplicate apn's from mbpi

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Let the user/connection-manager decide what to do with duplicate apn
entries instead of bailing out with an error.
---
 plugins/provision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/provision.c b/plugins/provision.c
index 99c299eb..aa0b05e4 100644
--- a/plugins/provision.c
+++ b/plugins/provision.c
@@ -50,7 +50,7 @@ static int provision_get_settings(const char *mcc, const char *mnc,
 
 	DBG("Provisioning for MCC %s, MNC %s, SPN '%s'", mcc, mnc, spn);
 
-	apns = mbpi_lookup_apn(mcc, mnc, FALSE, &error);
+	apns = mbpi_lookup_apn(mcc, mnc, TRUE, &error);
 	if (apns == NULL) {
 		if (error != NULL) {
 			ofono_error("%s", error->message);
-- 
2.20.1

Re: [PATCH] gprs: fix allocation of context id

[Denis Kenzior]

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

Hi Martin,

On 01/02/2019 05:50 AM, Martin Hundebøll wrote:
> After the convertion to l_uintset, the creation of new contexts fails
> due to a range error being returned from l_uintset_find_unused().
> 
> The error happens because the uinset is created with a min-value of 1,
> but the start-value passed to l_uintset_find_unused() is initialized as
> 0.

Whoops, my bad.  I forgot how my own API works.

> 
> Fix this by passing a start-value just past the last allocated context
> id.

Won't this return an error if the last_context_id is at max?  I think 
the right fix is to back to the original logic, where

if (!gprs->last_context_id)
	id = l_uintset_find_unused_min();
else
	id = l_uintset_find-unused();

> ---
>   src/gprs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Regards,
-Denis

Re: [PATCH] provision: allow duplicate apn's from mbpi

[Denis Kenzior]

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

Hi Martin,

On 01/02/2019 05:50 AM, Martin Hundebøll wrote:
> Let the user/connection-manager decide what to do with duplicate apn
> entries instead of bailing out with an error.
> ---
>   plugins/provision.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

While seemingly trivial, this actually has a huge impact on user 
experience.  The problem is that there's no good meta-data for the user 
to make the decision as to which service is the one they want.

Also, I'm not sure how this impacts connman?  In theory it is supposed 
to auto-activate the internet context, and if there are multiple one of 
these, I'm not sure if it gets confused?

The typical guidance is that if a database contains potential 
duplicates, then a UI Wizard is needed to guide the user to choose the 
appropriate one and provision the context that way.

Alternatively, provide a provisioning database without conflicts, 
perhaps specific to the users / operators you're targeting.

Regards,
-Denis

Re: [PATCH] provision: allow duplicate apn's from mbpi

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Hi Denis,

On 02/01/2019 19.36, Denis Kenzior wrote:
> On 01/02/2019 05:50 AM, Martin Hundebøll wrote:
>> Let the user/connection-manager decide what to do with duplicate apn
>> entries instead of bailing out with an error.
>> ---
>>   plugins/provision.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> While seemingly trivial, this actually has a huge impact on user 
> experience.  The problem is that there's no good meta-data for the user 
> to make the decision as to which service is the one they want.

Yes. The lack of meta-data makes it difficult to make a good decision. 
In the case of the mobile-broadband-provider-info the <name> element 
from the parent provider element would be helpful:

diff --git a/plugins/mbpi.c b/plugins/mbpi.c
index 433f1b55..69ffea8a 100644
--- a/plugins/mbpi.c
+++ b/plugins/mbpi.c
@@ -56,6 +56,7 @@ struct gsm_data {
         GSList *apns;
         gboolean match_found;
         gboolean allow_duplicates;
+       char *current_name;
  };

  struct cdma_data {
@@ -401,6 +402,9 @@ static void gsm_end(GMarkupParseContext *context, 
const gchar *element_name,
         if (!ap->username || !ap->password)
                 ap->auth_method = OFONO_GPRS_AUTH_METHOD_NONE;

+       if (!ap->name && gsm->current_name)
+               ap->name = g_strdup(gsm->current_name);
+
         if (gsm->allow_duplicates == FALSE) {
                 GSList *l;

@@ -503,7 +507,12 @@ 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->current_name);
+               gsm->current_name = NULL;
+               g_markup_parse_context_push(context, &text_parser,
+                                               &gsm->current_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"))
@@ -514,9 +523,16 @@ static void toplevel_gsm_end(GMarkupParseContext 
*context,
                                         const gchar *element_name,
                                         gpointer userdata, GError **error)
  {
+       struct gsm_data *gsm = userdata;
+
         if (g_str_equal(element_name, "gsm") ||
-                       g_str_equal(element_name, "cdma"))
+                       g_str_equal(element_name, "cdma") ||
+                       g_str_equal(element_name, "name"))
                 g_markup_parse_context_pop(context);
+       else if (g_str_equal(element_name, "provider")) {
+               g_free(gsm->current_name);
+               gsm->current_name = NULL;
+       }
  }

  static const GMarkupParser toplevel_gsm_parser = {


> Also, I'm not sure how this impacts connman?  In theory it is supposed 
> to auto-activate the internet context, and if there are multiple one of 
> these, I'm not sure if it gets confused?

I haven't looked at connman (yet). Couldn't connman create an empty 
internet context if it gets confused? (just like the one ofono creates 
when provision fails.)

> The typical guidance is that if a database contains potential 
> duplicates, then a UI Wizard is needed to guide the user to choose the 
> appropriate one and provision the context that way.

Yes, but wouldn't that require access to the duplicate entries to be 
able to create the wizard?

> Alternatively, provide a provisioning database without conflicts, 
> perhaps specific to the users / operators you're targeting.

That removes the convenience from relying on the 
mobile-broadband-provider-info maintained by upstream.

-- 
Kind regards,
Martin Hundebøll
Embedded Linux Consultant

+45 61 65 54 61
martin(a)geanix.com

Geanix IVS
https://geanix.com
DK39600706

Re: [PATCH] provision: allow duplicate apn's from mbpi

[Denis Kenzior]

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

Hi Martin,

On 01/04/2019 08:03 AM, Martin Hundebøll wrote:
> Hi Denis,
> 
> On 02/01/2019 19.36, Denis Kenzior wrote:
>> On 01/02/2019 05:50 AM, Martin Hundebøll wrote:
>>> Let the user/connection-manager decide what to do with duplicate apn
>>> entries instead of bailing out with an error.
>>> ---
>>>   plugins/provision.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> While seemingly trivial, this actually has a huge impact on user 
>> experience.  The problem is that there's no good meta-data for the 
>> user to make the decision as to which service is the one they want.
> 
> Yes. The lack of meta-data makes it difficult to make a good decision. 
> In the case of the mobile-broadband-provider-info the <name> element 
> from the parent provider element would be helpful:
> 
> diff --git a/plugins/mbpi.c b/plugins/mbpi.c
> index 433f1b55..69ffea8a 100644
> --- a/plugins/mbpi.c
> +++ b/plugins/mbpi.c
> @@ -56,6 +56,7 @@ struct gsm_data {
>          GSList *apns;
>          gboolean match_found;
>          gboolean allow_duplicates;
> +       char *current_name;
>   };
> 
>   struct cdma_data {
> @@ -401,6 +402,9 @@ static void gsm_end(GMarkupParseContext *context, 
> const gchar *element_name,
>          if (!ap->username || !ap->password)
>                  ap->auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
> 
> +       if (!ap->name && gsm->current_name)
> +               ap->name = g_strdup(gsm->current_name);
> +
>          if (gsm->allow_duplicates == FALSE) {
>                  GSList *l;
> 
> @@ -503,7 +507,12 @@ 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->current_name);
> +               gsm->current_name = NULL;
> +               g_markup_parse_context_push(context, &text_parser,
> +                                               &gsm->current_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"))
> @@ -514,9 +523,16 @@ static void toplevel_gsm_end(GMarkupParseContext 
> *context,
>                                          const gchar *element_name,
>                                          gpointer userdata, GError **error)
>   {
> +       struct gsm_data *gsm = userdata;
> +
>          if (g_str_equal(element_name, "gsm") ||
> -                       g_str_equal(element_name, "cdma"))
> +                       g_str_equal(element_name, "cdma") ||
> +                       g_str_equal(element_name, "name"))
>                  g_markup_parse_context_pop(context);
> +       else if (g_str_equal(element_name, "provider")) {
> +               g_free(gsm->current_name);
> +               gsm->current_name = NULL;
> +       }
>   }
> 
>   static const GMarkupParser toplevel_gsm_parser = {
> 

I don't really mind taking a patch like this since it at least gives 
some basic name to the context if the <name> tag is missing inside the 
<apn> tag.  But...  I don't see how this helps you at all?  If you have 
duplicates, then you just get both contexts named the same way.  Maybe 
you're lucky and one context has a name and the other can use the 
provider name.  But still, not great user experience.

Also note that mbpi doesn't really make it clear whether any of these 
names are localized or need to be translated.  oFono doesn't do 
translations, any printable strings are assumed to be localized by the SIM.

I guess you can tell that I'm no fan of mbpi.  If you're doing something 
serious, you're much better off looking into using something else.

> 
>> Also, I'm not sure how this impacts connman?  In theory it is supposed 
>> to auto-activate the internet context, and if there are multiple one 
>> of these, I'm not sure if it gets confused?
> 
> I haven't looked at connman (yet). Couldn't connman create an empty 
> internet context if it gets confused? (just like the one ofono creates 
> when provision fails.)

I don't think so?  But this is a conversation you need to have with the 
ConnMan folks.

> 
>> The typical guidance is that if a database contains potential 
>> duplicates, then a UI Wizard is needed to guide the user to choose the 
>> appropriate one and provision the context that way.
> 
> Yes, but wouldn't that require access to the duplicate entries to be 
> able to create the wizard?

No, just have the wizard parse mbpi directly.

> 
>> Alternatively, provide a provisioning database without conflicts, 
>> perhaps specific to the users / operators you're targeting.
> 
> That removes the convenience from relying on the 
> mobile-broadband-provider-info maintained by upstream.
> 

Not a big loss in my opinion ;)

Regards,
-Denis

Re: [PATCH] provision: allow duplicate apn's from mbpi

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Hi Denis,

On 04/01/2019 22.03, Denis Kenzior wrote:
> On 01/04/2019 08:03 AM, Martin Hundebøll wrote:
>> On 02/01/2019 19.36, Denis Kenzior wrote:
>>> On 01/02/2019 05:50 AM, Martin Hundebøll wrote:
>>>> Let the user/connection-manager decide what to do with duplicate apn
>>>> entries instead of bailing out with an error.
>>>> ---
>>>>   plugins/provision.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> While seemingly trivial, this actually has a huge impact on user 
>>> experience.  The problem is that there's no good meta-data for the 
>>> user to make the decision as to which service is the one they want.
>>
>> Yes. The lack of meta-data makes it difficult to make a good decision. 
>> In the case of the mobile-broadband-provider-info the <name> element 
>> from the parent provider element would be helpful:
>>
>> diff --git a/plugins/mbpi.c b/plugins/mbpi.c
>> index 433f1b55..69ffea8a 100644
>> --- a/plugins/mbpi.c
>> +++ b/plugins/mbpi.c
>> @@ -56,6 +56,7 @@ struct gsm_data {
>>          GSList *apns;
>>          gboolean match_found;
>>          gboolean allow_duplicates;
>> +       char *current_name;
>>   };
>>
>>   struct cdma_data {
>> @@ -401,6 +402,9 @@ static void gsm_end(GMarkupParseContext *context, 
>> const gchar *element_name,
>>          if (!ap->username || !ap->password)
>>                  ap->auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
>>
>> +       if (!ap->name && gsm->current_name)
>> +               ap->name = g_strdup(gsm->current_name);
>> +
>>          if (gsm->allow_duplicates == FALSE) {
>>                  GSList *l;
>>
>> @@ -503,7 +507,12 @@ 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->current_name);
>> +               gsm->current_name = NULL;
>> +               g_markup_parse_context_push(context, &text_parser,
>> +                                               &gsm->current_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"))
>> @@ -514,9 +523,16 @@ static void toplevel_gsm_end(GMarkupParseContext 
>> *context,
>>                                          const gchar *element_name,
>>                                          gpointer userdata, GError 
>> **error)
>>   {
>> +       struct gsm_data *gsm = userdata;
>> +
>>          if (g_str_equal(element_name, "gsm") ||
>> -                       g_str_equal(element_name, "cdma"))
>> +                       g_str_equal(element_name, "cdma") ||
>> +                       g_str_equal(element_name, "name"))
>>                  g_markup_parse_context_pop(context);
>> +       else if (g_str_equal(element_name, "provider")) {
>> +               g_free(gsm->current_name);
>> +               gsm->current_name = NULL;
>> +       }
>>   }
>>
>>   static const GMarkupParser toplevel_gsm_parser = {
>>
> 
> I don't really mind taking a patch like this since it at least gives 
> some basic name to the context if the <name> tag is missing inside the 
> <apn> tag.  But...  I don't see how this helps you at all?  If you have 
> duplicates, then you just get both contexts named the same way.  Maybe 
> you're lucky and one context has a name and the other can use the 
> provider name.  But still, not great user experience.

In my case there are multiple providers (and thus names) with the same 
mcc/mnc (238/01), and the cases of multiple apns for the same provider 
are mostly internet/mms, so it is still an improvement:

https://github.com/GNOME/mobile-broadband-provider-info/blob/master/serviceproviders.xml#L3830

> Also note that mbpi doesn't really make it clear whether any of these 
> names are localized or need to be translated.  oFono doesn't do 
> translations, any printable strings are assumed to be localized by the SIM.

I expect mbpi to use the locale of the country the element is organized 
below. This is at least what is done for e.g. Iran:

https://github.com/GNOME/mobile-broadband-provider-info/blob/master/serviceproviders.xml#L7082

Of course this doesn't fix the case with multiple locales for the same 
country, or if I should happen to be in Iran :)

> I guess you can tell that I'm no fan of mbpi.  If you're doing something 
> serious, you're much better off looking into using something else.

Fair enough.

>>
>>> Also, I'm not sure how this impacts connman?  In theory it is 
>>> supposed to auto-activate the internet context, and if there are 
>>> multiple one of these, I'm not sure if it gets confused?
>>
>> I haven't looked at connman (yet). Couldn't connman create an empty 
>> internet context if it gets confused? (just like the one ofono creates 
>> when provision fails.)
> 
> I don't think so?  But this is a conversation you need to have with the 
> ConnMan folks.

ConnMan is out of my scope here.

>>> The typical guidance is that if a database contains potential 
>>> duplicates, then a UI Wizard is needed to guide the user to choose 
>>> the appropriate one and provision the context that way.
>>
>> Yes, but wouldn't that require access to the duplicate entries to be 
>> able to create the wizard?
> 
> No, just have the wizard parse mbpi directly.

Point taken. This is also what NetworkManager/ModemManager does, I guess.

>>> Alternatively, provide a provisioning database without conflicts, 
>>> perhaps specific to the users / operators you're targeting.
>>
>> That removes the convenience from relying on the 
>> mobile-broadband-provider-info maintained by upstream.
>>
> 
> Not a big loss in my opinion ;)

Okay.

In any case: I was simply playing with this for development/testing 
purposes, so I can live with failure on duplicate entries.

I can send the mbpi name diff as a regular patch if you will take it, 
and drop my original patch.

// Martin

Re: [PATCH] provision: allow duplicate apn's from mbpi

[Denis Kenzior]

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

Hi Martin,

> I can send the mbpi name diff as a regular patch if you will take it,  > and drop my original patch.
Sure, I can take it as long as it doesn't mess with the duplicate 
handling behavior.

Regards,
-Denis