On Mon, Mar 7, 2016 at 5:13 PM, Carlo Lobrano <[email protected]> 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 _______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
