Re: [MM] [PATCH] bearer: deny disconnect request if there is a cancel outstanding

[Thieu Le <thieule chromium org>]

Hi Aleksander,

I've tried your patch below and mm-next did not hang during my test
runs.  Although it didn't happen during my test passes, I am wondering
if the bearer DISCONNECTING status can be overwritten if the modem
changes state after the call to mm_bearer_disconnect()?

If there is no possibility of overwriting the DISCONNECTING status,
then we can go with your approach.

Thanks,
Thieu


On Mon, Jul 30, 2012 at 5:56 AM, Aleksander Morgado
<aleksander lanedo com> wrote:
> Hey Ben & Thieu,
>
>> ---
>>  src/mm-bearer.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/mm-bearer.c b/src/mm-bearer.c
>> index 3cce948..3b1be8c 100644
>> --- a/src/mm-bearer.c
>> +++ b/src/mm-bearer.c
>> @@ -450,7 +450,8 @@ mm_bearer_disconnect (MMBearer *self,
>>      }
>>
>>      /* If already disconnecting, return error, don't allow a second request. */
>> -    if (self->priv->status == MM_BEARER_STATUS_DISCONNECTING) {
>> +    if (self->priv->status == MM_BEARER_STATUS_DISCONNECTING ||
>> +        self->priv->disconnect_signal_handler) {
>>          g_simple_async_result_set_error (
>>              simple,
>>              MM_CORE_ERROR,
>>
>
> Just wondering if it's not better to change the bearer state to
> 'disconnecting' when we cancel the connection attempt, something like:
>
> diff --git a/src/mm-bearer.c b/src/mm-bearer.c
> index 3cce948..d8e681f 100644
> --- a/src/mm-bearer.c
> +++ b/src/mm-bearer.c
> @@ -466,6 +466,8 @@ mm_bearer_disconnect (MMBearer *self,
>      /* If currently connecting, try to cancel that operation, and wait
> to get
>       * disconnected. */
>      if (self->priv->status == MM_BEARER_STATUS_CONNECTING) {
> +        bearer_update_status (self, MM_BEARER_STATUS_DISCONNECTING);
> +
>          /* We MUST ensure that we get to DISCONNECTED */
>          g_cancellable_cancel (self->priv->connect_cancellable);
>
>
> I believe that it should work; could you guys test that?
>
> Cheers!
>
> --
> Aleksander