[PATCH] gprs: Use "internet" apn for auto-created context

[PATCH] gprs: Use "internet" apn for auto-created context

[Slava Monich]

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

Such an access point has a pretty good chance to actually work.
---
 src/gprs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index e379f7b..01ff875 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -2992,8 +2992,13 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
 	const char *path = __ofono_atom_get_path(gprs->atom);
 
-	if (gprs->contexts == NULL) /* Automatic provisioning failed */
-		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
+	if (gprs->contexts == NULL) { /* Automatic provisioning failed */
+		struct pri_context *context = add_context(gprs, NULL,
+					OFONO_GPRS_CONTEXT_TYPE_INTERNET);
+		if (context) {
+			strcpy(context->context.apn, "internet");
+		}
+	}
 
 	if (!g_dbus_register_interface(conn, path,
 					OFONO_CONNECTION_MANAGER_INTERFACE,
-- 
1.8.3.2

Re: [PATCH] gprs: Use "internet" apn for auto-created context

[Denis Kenzior]

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

Hi Slava,

On 01/16/2014 08:43 AM, Slava Monich wrote:
> Such an access point has a pretty good chance to actually work.
> ---
>   src/gprs.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index e379f7b..01ff875 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -2992,8 +2992,13 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
>   	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
>   	const char *path = __ofono_atom_get_path(gprs->atom);
>
> -	if (gprs->contexts == NULL) /* Automatic provisioning failed */
> -		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
> +	if (gprs->contexts == NULL) { /* Automatic provisioning failed */
> +		struct pri_context *context = add_context(gprs, NULL,
> +					OFONO_GPRS_CONTEXT_TYPE_INTERNET);
> +		if (context) {
> +			strcpy(context->context.apn, "internet");
> +		}
> +	}

Nope.  This doesn't belong in the core.  If you want to guess wildly, 
please do so in a provisioning plugin.  That is what they're there for. 
  You can have multiple provisioning plugins by the way.

>
>   	if (!g_dbus_register_interface(conn, path,
>   					OFONO_CONNECTION_MANAGER_INTERFACE,
>

Regards,
-Denis

Re: [PATCH] gprs: Use "internet" apn for auto-created context

[Slava Monich]

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

Hi Denis,

Sorry to bother you, but I don't quite get the logic. There's something 
that needs to be done in case if ALL provisioning plugins have run and 
failed. How could that be a part of a provisioning plugin? They all have 
already run and failed by definition of the problem.

And another thing. If context initialization belongs exclusively to the 
provisioning plugins then what is this add_context() call doing there? 
And why INTERNET context is created but, say, MMS is not? Isn't that a 
wild guess too? If so then I have found exactly the right place for 
another wild guess. Or am I missing something?

I don't want to waste your time as well as mine by submitting patches 
that get rejected but in order to do so I have to understand why patches 
get rejected.

Regards,
-Slava


> Hi Slava,
>
> On 01/16/2014 08:43 AM, Slava Monich wrote:
>> Such an access point has a pretty good chance to actually work.
>> ---
>>   src/gprs.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gprs.c b/src/gprs.c
>> index e379f7b..01ff875 100644
>> --- a/src/gprs.c
>> +++ b/src/gprs.c
>> @@ -2992,8 +2992,13 @@ static void ofono_gprs_finish_register(struct 
>> ofono_gprs *gprs)
>>       struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
>>       const char *path = __ofono_atom_get_path(gprs->atom);
>>
>> -    if (gprs->contexts == NULL) /* Automatic provisioning failed */
>> -        add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
>> +    if (gprs->contexts == NULL) { /* Automatic provisioning failed */
>> +        struct pri_context *context = add_context(gprs, NULL,
>> +                    OFONO_GPRS_CONTEXT_TYPE_INTERNET);
>> +        if (context) {
>> +            strcpy(context->context.apn, "internet");
>> +        }
>> +    }
>
> Nope.  This doesn't belong in the core.  If you want to guess wildly, 
> please do so in a provisioning plugin.  That is what they're there 
> for.  You can have multiple provisioning plugins by the way.
>
>>
>>       if (!g_dbus_register_interface(conn, path,
>>                       OFONO_CONNECTION_MANAGER_INTERFACE,
>>
>
> Regards,
> -Denis
>

Re: [PATCH] gprs: Use "internet" apn for auto-created context

[Denis Kenzior]

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

Hi Slava,

A gentle reminder to please not top-post on this mailing list.

On 01/16/2014 02:52 PM, Slava Monich wrote:
> Hi Denis,
>
> Sorry to bother you, but I don't quite get the logic. There's something
> that needs to be done in case if ALL provisioning plugins have run and
> failed. How could that be a part of a provisioning plugin? They all have
> already run and failed by definition of the problem.

We do; we create an empty context to signify that provisioning has 
failed.  I do realize you mean well, but you're trying to wildly guess 
(at least that is how I read the commit description) and breaking 
existing behavior.

>
> And another thing. If context initialization belongs exclusively to the
> provisioning plugins then what is this add_context() call doing there?
> And why INTERNET context is created but, say, MMS is not? Isn't that a
> wild guess too? If so then I have found exactly the right place for
> another wild guess. Or am I missing something?

You are :)  The special context is not meant to be used, and is by 
design meant to always fail.

An empty APN is by definition invalid, so it is used as a 'special' flag 
value.  The 'special' empty APN context is used by various oFono UIs to 
start a user-guided provisioning process.  If you mess with this 
behavior, you will break those UIs.

Why only an internet context is created is largely historical.  This 
behavior probably predates MMS contexts.

>
> I don't want to waste your time as well as mine by submitting patches
> that get rejected but in order to do so I have to understand why patches
> get rejected.

Completely understandable.  Please feel free to start a conversation on 
the mailing list or the IRC channel whenever you feel you need more 
information.

Regards,
-Denis

Re: [PATCH] gprs: Use "internet" apn for auto-created context

[Slava Monich]

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

Hi Denis,

> Hi Slava,
>
> A gentle reminder to please not top-post on this mailing list.
>
> On 01/16/2014 02:52 PM, Slava Monich wrote:
>> Hi Denis,
>>
>> Sorry to bother you, but I don't quite get the logic. There's something
>> that needs to be done in case if ALL provisioning plugins have run and
>> failed. How could that be a part of a provisioning plugin? They all have
>> already run and failed by definition of the problem.
>
> We do; we create an empty context to signify that provisioning has 
> failed.  I do realize you mean well, but you're trying to wildly guess 
> (at least that is how I read the commit description) and breaking 
> existing behavior.
>

How can you tell an empty context created by a provisioning plugin from 
an empty context created by gprs.c after all provisioning plugins have 
failed? Or from the access point edited by the user in such a way that 
it looks exactly like the empty context created in gprs.c? You can't. 
They are all identical. Empty context is not a reliable indication of a 
provisioning failure.

A better, unambiguous indication would be to not create any contexts at 
all. Actually, I was assuming that it was the case and was quite 
surprised to find out that it was ofono creating the default context.

It's surely not breaking anything for Jolla. It would only improve 
chances of GPRS access to work out of the box. But of course in theory 
it may break something on some other platform. Any change, even an 
obvious bug fix can break existing behavior.

Finally, about 15% of all GPRS Internet access points in the world are 
called "internet" and have no user name or password. It's by far the 
most common use case. So it's not a wild guess at all. It's real life 
experience.

Regards,
-Slava

Re: [PATCH] gprs: Use "internet" apn for auto-created context

[Denis Kenzior]

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

Hi Slava,

 > How can you tell an empty context created by a provisioning plugin from
> an empty context created by gprs.c after all provisioning plugins have
> failed? Or from the access point edited by the user in such a way that
> it looks exactly like the empty context created in gprs.c? You can't.
> They are all identical. Empty context is not a reliable indication of a
> provisioning failure.
>

If your provisioning plugin returns bogus data, then the provisioning 
plugin is broken and needs to be fixed.  What exactly are you arguing?

If people start deploying plugins that return bogus / invalid 
information, then the core can validate the provisioning information 
just like it validates the D-Bus API.

> A better, unambiguous indication would be to not create any contexts at
> all. Actually, I was assuming that it was the case and was quite
> surprised to find out that it was ofono creating the default context.

No, it is not.  All contexts can be removed by the application, this is 
not an 'impossible' condition.  A context with an empty APN cannot be 
created programmatically.

>
> It's surely not breaking anything for Jolla. It would only improve
> chances of GPRS access to work out of the box. But of course in theory
> it may break something on some other platform. Any change, even an
> obvious bug fix can break existing behavior.
>
> Finally, about 15% of all GPRS Internet access points in the world are
> called "internet" and have no user name or password. It's by far the
> most common use case. So it's not a wild guess at all. It's real life
> experience.

And you can easily implement this behavior as a plugin if you so desire.

Regards,
-Denis