[PATCH 1/1] Fix primary context active/deactive result d-bus reply missing issue

[PATCH 1/1] Fix primary context active/deactive result d-bus reply missing issue

[martin.xu]

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

From: Martin Xu <martin.xu@intel.com>

If user offline modem when the gprs_context is in enabling or disabling
state(activating/disactivating), the GPRS primary context activate/disactivate
result will not be replied to the corresponding application.
---
 drivers/atmodem/gprs-context.c |    7 +++++++
 src/gprs.c                     |    5 +++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index ba95cbf..bfbd2c5 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -301,6 +301,13 @@ static void at_gprs_context_remove(struct ofono_gprs_context *gc)
 
 	if (gcd->state != STATE_IDLE && gcd->ppp) {
 		g_at_ppp_unref(gcd->ppp);
+
+		if (gcd->state == STATE_ENABLING)
+			CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+
+		if (gcd->state == STATE_DISABLING)
+			CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+
 		g_at_chat_resume(gcd->chat);
 	}
 
diff --git a/src/gprs.c b/src/gprs.c
index deffeb8..99cb55d 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -351,6 +351,9 @@ static struct pri_context *gprs_context_by_path(struct ofono_gprs *gprs,
 
 static void context_settings_free(struct context_settings *settings)
 {
+	if (settings == NULL)
+		return;
+
 	if (settings->ipv4) {
 		g_free(settings->ipv4->ip);
 		g_free(settings->ipv4->netmask);
@@ -736,6 +739,8 @@ static void pri_reset_context_settings(struct pri_context *ctx)
 		return;
 
 	settings = ctx->context_driver->settings;
+	if (settings == NULL)
+		return;
 
 	interface = settings->interface;
 	settings->interface = NULL;
-- 
1.7.2.2

Re: [PATCH 1/1] Fix primary context active/deactive result d-bus reply missing issue

[Denis Kenzior]

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

Hi Martin,

On 04/27/2011 02:04 AM, martin.xu(a)intel.com wrote:
> From: Martin Xu <martin.xu@intel.com>
> 
> If user offline modem when the gprs_context is in enabling or disabling
> state(activating/disactivating), the GPRS primary context activate/disactivate
> result will not be replied to the corresponding application.
> ---
>  drivers/atmodem/gprs-context.c |    7 +++++++
>  src/gprs.c                     |    5 +++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index ba95cbf..bfbd2c5 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -301,6 +301,13 @@ static void at_gprs_context_remove(struct ofono_gprs_context *gc)
>  
>  	if (gcd->state != STATE_IDLE && gcd->ppp) {
>  		g_at_ppp_unref(gcd->ppp);
> +
> +		if (gcd->state == STATE_ENABLING)
> +			CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> +
> +		if (gcd->state == STATE_DISABLING)
> +			CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> +

The biggest problem with doing it this way is that the context's
unregister function has already been called.  When this is called we're
actually in the process of removing the context, hence why you require
the hacks in the core below.  We also don't emit the PropertyChanged
signal properly if we do it this way.

There might be a couple ways to fix this:

- Make a specific remove function for the ppp context driver.  This will
call the callbacks as you do above and then remove the driver.

- Handle all of this inside the core.

>  		g_at_chat_resume(gcd->chat);
>  	}
>  
> diff --git a/src/gprs.c b/src/gprs.c
> index deffeb8..99cb55d 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -351,6 +351,9 @@ static struct pri_context *gprs_context_by_path(struct ofono_gprs *gprs,
>  
>  static void context_settings_free(struct context_settings *settings)
>  {
> +	if (settings == NULL)
> +		return;
> +
>  	if (settings->ipv4) {
>  		g_free(settings->ipv4->ip);
>  		g_free(settings->ipv4->netmask);
> @@ -736,6 +739,8 @@ static void pri_reset_context_settings(struct pri_context *ctx)
>  		return;
>  
>  	settings = ctx->context_driver->settings;
> +	if (settings == NULL)
> +		return;
>  
>  	interface = settings->interface;
>  	settings->interface = NULL;

Regards,
-Denis

Re: [PATCH 1/1] Fix primary context active/deactive result d-bus reply missing issue

[Denis Kenzior]

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

Hi Martin,

On 04/27/2011 02:04 AM, martin.xu(a)intel.com wrote:
> From: Martin Xu <martin.xu@intel.com>
> 
> If user offline modem when the gprs_context is in enabling or disabling
> state(activating/disactivating), the GPRS primary context activate/disactivate
> result will not be replied to the corresponding application.
> ---
>  drivers/atmodem/gprs-context.c |    7 +++++++
>  src/gprs.c                     |    5 +++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 

Please check commit 4714d8199aacf0ba6be777a70e01b3016ba2aa5f, this might
partly fix this issue.

Regards,
-Denis

RE: [PATCH 1/1] Fix primary context active/deactive result d-bus reply missing issue

[Xu, Martin]

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

Denis:
> -----Original Message-----
> From: Denis Kenzior [mailto:denkenz(a)gmail.com]
> Sent: Thursday, May 12, 2011 2:31 AM
> To: ofono(a)ofono.org
> Cc: Xu, Martin
> Subject: Re: [PATCH 1/1] Fix primary context active/deactive result d-bus reply
> missing issue
> 
> Hi Martin,
> 
> On 04/27/2011 02:04 AM, martin.xu(a)intel.com wrote:
> > From: Martin Xu <martin.xu@intel.com>
> >
> > If user offline modem when the gprs_context is in enabling or disabling
> > state(activating/disactivating), the GPRS primary context activate/disactivate
> > result will not be replied to the corresponding application.
> > ---
> >  drivers/atmodem/gprs-context.c |    7 +++++++
> >  src/gprs.c                     |    5 +++++
> >  2 files changed, 12 insertions(+), 0 deletions(-)
> >
> 
> Please check commit 4714d8199aacf0ba6be777a70e01b3016ba2aa5f, this
> might
> partly fix this issue.
Thanks. :-)
The patch only fix the issue partly, we are continue to test and review the code and try to find the final solution.