[PATCH 1/2] gprs: pri_set_apn not exclusively a DBus method

[PATCH 1/2] gprs: pri_set_apn not exclusively a DBus method

[Jonas Bonn]

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

pri_set_apn is called by ofono_gprs_cid_activated() which is not a DBus
method; as such, there is no DBus return message.  Attempting to create
the return message with a NULL 'msg' may cause the DBus library to
call abort().
---
 src/gprs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index c5e7709..ed2131e 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1030,8 +1030,12 @@ static DBusMessage *pri_set_apn(struct pri_context *ctx, DBusConnection *conn,
 	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
 		return __ofono_error_invalid_format(msg);
 
-	if (g_str_equal(apn, ctx->context.apn))
-		return dbus_message_new_method_return(msg);
+	if (g_str_equal(apn, ctx->context.apn)) {
+		if (msg)
+			return dbus_message_new_method_return(msg);
+		else
+			return NULL;
+	}
 
 	if (is_valid_apn(apn) == FALSE)
 		return __ofono_error_invalid_format(msg);
-- 
2.9.3

[PATCH 2/2] qmimodem: set APN for LTE default bearer

[Jonas Bonn]

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

Apparently, an empty APN in an ofono context means that that the context
cannot be activated.  connman definitely interprets it this way.

This patch sets a default name of "automatic" for the default bearer if
no other LTE APN is supplied (which is currently the case as the LTE
atom is not in place yet).  Without this, connman happily ignores the
context, even though it has been activated by ofono.
---
 drivers/qmimodem/gprs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
index ac52b19..a80d55f 100644
--- a/drivers/qmimodem/gprs.c
+++ b/drivers/qmimodem/gprs.c
@@ -83,7 +83,7 @@ static int handle_ss_info(struct qmi_result *result, struct ofono_gprs *gprs)
 			/* FIXME: query default profile number and APN
 			 * instead of assuming profile 1 and ""
 			 */
-			ofono_gprs_cid_activated(gprs, 1 , "");
+			ofono_gprs_cid_activated(gprs, 1 , "automatic");
 		}
 
 	return status;
-- 
2.9.3

Re: [PATCH 1/2] gprs: pri_set_apn not exclusively a DBus method

[Denis Kenzior]

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

Hi Jonas,

On 05/04/2017 08:24 AM, Jonas Bonn wrote:
> pri_set_apn is called by ofono_gprs_cid_activated() which is not a DBus
> method; as such, there is no DBus return message.  Attempting to create
> the return message with a NULL 'msg' may cause the DBus library to
> call abort().
> ---
>  src/gprs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index c5e7709..ed2131e 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -1030,8 +1030,12 @@ static DBusMessage *pri_set_apn(struct pri_context *ctx, DBusConnection *conn,
>  	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
>  		return __ofono_error_invalid_format(msg);
>
> -	if (g_str_equal(apn, ctx->context.apn))
> -		return dbus_message_new_method_return(msg);
> +	if (g_str_equal(apn, ctx->context.apn)) {
> +		if (msg)
> +			return dbus_message_new_method_return(msg);
> +		else
> +			return NULL;
> +	}

This doesn't really help as __ofono_error still uses dbus_message_new... 
with a NULL argument.

I suspect that the code in this function likely needs to be duplicated 
for the cid_activated case.

>
>  	if (is_valid_apn(apn) == FALSE)
>  		return __ofono_error_invalid_format(msg);
>

Regards,
-Denis

Re: [PATCH 2/2] qmimodem: set APN for LTE default bearer

[Denis Kenzior]

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

Hi Jonas,

On 05/04/2017 08:24 AM, Jonas Bonn wrote:
> Apparently, an empty APN in an ofono context means that that the context
> cannot be activated.  connman definitely interprets it this way.
>
> This patch sets a default name of "automatic" for the default bearer if
> no other LTE APN is supplied (which is currently the case as the LTE
> atom is not in place yet).  Without this, connman happily ignores the
> context, even though it has been activated by ofono.
> ---
>  drivers/qmimodem/gprs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Regards,
-Denis

[PATCH] Fix crash in pri_set_apn when using an LTE SIM

[Christophe Ronco]

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

I use a MC7304 modem.
As Jonas said, ofonod crashes when using an LTE SIM.
I didn't duplicate pri_set_apn code as Denis proposed,
I made it support a NULL msg parameter.


Christophe Ronco (1):
  gprs: pri_set_apn not exclusively a DBus method

 src/gprs.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH] gprs: pri_set_apn not exclusively a DBus method

[Christophe Ronco]

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

pri_set_apn is called by ofono_gprs_cid_activated() when attached
using LTE technology.
This patch avoid calling DBUS functions in this case. Without
this patch we have a crash in ofono process in this case.

diff --git a/src/gprs.c b/src/gprs.c
index c5e7709..9c6d282 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1027,14 +1027,28 @@ static DBusMessage *pri_set_apn(struct pri_context *ctx, DBusConnection *conn,
 {
 	GKeyFile *settings = ctx->gprs->settings;
 
-	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
-		return __ofono_error_invalid_format(msg);
+	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH) {
+		if (msg)
+			return __ofono_error_invalid_format(msg);
 
-	if (g_str_equal(apn, ctx->context.apn))
-		return dbus_message_new_method_return(msg);
+		DBG("APN %s too long", apn);
+		return NULL;
+	}
 
-	if (is_valid_apn(apn) == FALSE)
-		return __ofono_error_invalid_format(msg);
+	if (g_str_equal(apn, ctx->context.apn)) {
+		if (msg)
+			return dbus_message_new_method_return(msg);
+
+		return NULL;
+	}
+
+	if (is_valid_apn(apn) == FALSE) {
+		if (msg)
+			return __ofono_error_invalid_format(msg);
+
+		DBG("APN %s invalid", apn);
+		return NULL;
+	}
 
 	strcpy(ctx->context.apn, apn);
 
-- 
2.7.4

Re: [PATCH] gprs: pri_set_apn not exclusively a DBus method

[Denis Kenzior]

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

Hi Christophe,

On 05/09/2017 09:52 AM, Christophe Ronco wrote:
> pri_set_apn is called by ofono_gprs_cid_activated() when attached
> using LTE technology.
> This patch avoid calling DBUS functions in this case. Without
> this patch we have a crash in ofono process in this case.
>
> diff --git a/src/gprs.c b/src/gprs.c
> index c5e7709..9c6d282 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -1027,14 +1027,28 @@ static DBusMessage *pri_set_apn(struct pri_context *ctx, DBusConnection *conn,
>  {
>  	GKeyFile *settings = ctx->gprs->settings;
>
> -	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
> -		return __ofono_error_invalid_format(msg);
> +	if (strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH) {
> +		if (msg)
> +			return __ofono_error_invalid_format(msg);
>
> -	if (g_str_equal(apn, ctx->context.apn))
> -		return dbus_message_new_method_return(msg);
> +		DBG("APN %s too long", apn);
> +		return NULL;
> +	}
>
> -	if (is_valid_apn(apn) == FALSE)
> -		return __ofono_error_invalid_format(msg);
> +	if (g_str_equal(apn, ctx->context.apn)) {
> +		if (msg)
> +			return dbus_message_new_method_return(msg);
> +
> +		return NULL;

Returning NULL on a success path doesn't really make any sense.

> +	}
> +
> +	if (is_valid_apn(apn) == FALSE) {
> +		if (msg)
> +			return __ofono_error_invalid_format(msg);
> +
> +		DBG("APN %s invalid", apn);
> +		return NULL;
> +	}
>
>  	strcpy(ctx->context.apn, apn);
>
>

Anyway, I have now fixed this in 
d9cb969dcff649d33f97ceddac57163dfc9602db.  Let me know if I broke something.

Regards,
-Denis

Re: [PATCH] gprs: pri_set_apn not exclusively a DBus method

[Jonas Bonn]

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

On 05/09/2017 06:37 PM, Denis Kenzior wrote:
> Anyway, I have now fixed this in 
> d9cb969dcff649d33f97ceddac57163dfc9602db.  Let me know if I broke 
> something.

Did you forget to push that?  ;)

/Jonas

Re: [PATCH] gprs: pri_set_apn not exclusively a DBus method

[Denis Kenzior]

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

Hi Jonas,

On 05/09/2017 01:08 PM, Jonas Bonn wrote:
> On 05/09/2017 06:37 PM, Denis Kenzior wrote:
>> Anyway, I have now fixed this in
>> d9cb969dcff649d33f97ceddac57163dfc9602db.  Let me know if I broke
>> something.
>
> Did you forget to push that?  ;)
>

Yes I did.  Sorry about that.  Pushed out now.

Regards,
-Denis