[PATCH] Fix Bug 93135 +CPMS emtpy parameter not always supported

[Aleksander Morgado]

On Mon, Mar 7, 2016 at 5:13 PM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
>> > +
>> > +
>> >
>> > +/*****************************************************************************/
>> >  /* Lock/unlock SMS storage (Messaging interface implementation helper)
>> >   *
>> >   * The basic commands to work with SMS storages play with AT+CPMS and
>> > three
>> > @@ -5373,8 +5449,9 @@ mm_broadband_modem_lock_sms_storages
>> > (MMBroadbandModem *self,
>> >          ctx->previous_mem1 = self->priv->current_sms_mem1_storage;
>> >          self->priv->mem1_storage_locked = TRUE;
>> >          self->priv->current_sms_mem1_storage = mem1;
>> > -        mem1_str = g_ascii_strup (mm_sms_storage_get_string
>> > (self->priv->current_sms_mem1_storage), -1);
>> >      }
>> > +    /* We always provide a non empty string parameter as mem1 */
>> > +    mem1_str = g_ascii_strup (mm_sms_storage_get_string
>> > (self->priv->current_sms_mem1_storage), -1);
>> >
>>
>> This is a bit confusing actually. The fact that we're passing the mem1
>> value when e.g. trying to lock mem2 doesn't mean that we can be
>> changing the mem1 while some other async logic got it locked before. I
>> think your code is ok, but I spent 30 mins convinced that it wasn't,
>> I'm not 100% sure yet :) The key point is that If mem1 is locked for
>> an operation, no other operation that wants to lock mem1 can be run
>> (ok); but we can still run operations that want to lock mem2 only (and
>> in that case we're just re-sending the already locked mem1 value). Can
>> you try to explain that in the comment?
>>
>
> I'm actually considering now the fact that I need a mem1_str only when
> mem2_str is not NULL.
>
> I could keep the "if (mem1 != MM_SMS_STORAGE_UNKNOWN)" as it was before, and
> do something like this
>
> if (mem2 != MM_SMS_STORAGE_UNKNOWN) {
>     ...
>     mem2_str = g_ascii_strup (mm_sms_storage_get_string
> (self->priv->current_sms_mem2_storage), -1);
>
>     if (mem1 == MM_SMS_STORAGE_UNKNOWN) {
>         /* Verbose explanation */
>         mem1_str = g_ascii_strup(mm_sms_storage_get_string
> (self->priv->current_sms_mem1_storage), -1);
>     }
> }
>
> This way the change will effect only the cases when mem2 only is to be
> locked and the verbose explanation should be more clear to understand.
>
> What do you think?

Much better and clearer in that way, yes

-- 
Aleksander
https://aleksander.es