Sure, I didn't see the other error. I'll update the patch Il venerdì 24 marzo 2017, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es">aleksander@aleksander.es</a>> ha scritto: > Hey Carlo, > > On Fri, Mar 24, 2017 at 12:48 PM, Carlo Lobrano <<a href="mailto:c.lobrano@gmail.com">c.lobrano@gmail.com</a>> wrote: >> Let modem_messaging_set_default_storage returns with error >> if current_sms_mem1_storage is MM_SMS_STORAGE_UNKNOWN >> >> --- >> >> I updated the patch according the code review, moreover I'd like >> to note a little difference respect the previous version: >> >> currently, even if +CPMS fails because of the wrong value for >> mem1, we do store "storage" as the current_sms_mem2_storage value. >> >> self->priv->current_sms_mem2_storage = storage; >> >> In this patch I preferred to change this behaviour and setting default >> storage for SMS mem2 only if we are actually going to set >> this value on the modem. >> > > That is a good change, yes. > >> What do you think about it? >> > > The same issue happens in mm_broadband_modem_lock_sms_storages(); > where we may end up using "self->priv->current_sms_mem1_storage" > without checking it is UNKNOWN first. Could you also update that > method with the same logic? i.e. return an error if we end up there > with UNKNOWN in "current_sms_mem1_storage". > > Also see comments below. > >> --- >> src/mm-broadband-modem.c | 29 +++++++++++++++++++---------- >> src/mm-iface-modem-messaging.c | 2 +- >> 2 files changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c >> index 17b3253..0bbf626 100644 >> --- a/src/mm-broadband-modem.c >> +++ b/src/mm-broadband-modem.c >> @@ -5590,22 +5590,31 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self, >> gchar *mem1_str; >> gchar *mem_str; >> >> - result = g_simple_async_result_new (G_OBJECT (self), >> - callback, >> - user_data, >> - modem_messaging_set_default_storage); >> + /* We provide the current sms storage for mem1 if not UNKNOWN */ >> + if (self->priv->current_sms_mem1_storage == MM_SMS_STORAGE_UNKNOWN) { >> + g_simple_async_report_error_in_idle (G_OBJECT (self), >> + callback, >> + user_data, >> + MM_CORE_ERROR, >> + MM_CORE_ERROR_INVALID_ARGS, >> + "Current SMS MEM1 storage is UNKNOWN (SIM busy?)"); >> + return; >> + } >> >> /* Set defaults as current */ >> self->priv->current_sms_mem2_storage = storage; >> >> - /* We provide the current sms storage for mem1 if not UNKNOWN */ >> mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1); >> - >> mem_str = g_ascii_strup (mm_sms_storage_get_string (storage), -1); >> - cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\",\"%s\"", >> - mem1_str ? mem1_str : "", >> - mem_str, >> - mem_str); >> + >> + cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\",\"%s\"", mem1_str, mem_str, mem_str); >> + >> + result = g_simple_async_result_new (G_OBJECT (self), >> + callback, >> + user_data, >> + modem_messaging_set_default_storage); >> + >> + > > Just a single whiteline here. > >> mm_base_modem_at_command (MM_BASE_MODEM (self), >> cmd, >> 3, >> diff --git a/src/mm-iface-modem-messaging.c b/src/mm-iface-modem-messaging.c >> index 0cff1f2..01a30f0 100644 >> --- a/src/mm-iface-modem-messaging.c >> +++ b/src/mm-iface-modem-messaging.c >> @@ -789,7 +789,7 @@ set_default_storage_ready (MMIfaceModemMessaging *self, >> GError *error = NULL; >> >> if (!MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (self)->set_default_storage_finish (self, res, &error)) { >> - mm_dbg ("Couldn't set default storage: '%s'", error->message); >> + mm_warn ("Could not set default storage: '%s'", error->message); >> g_error_free (error); >> } >> >> -- >> 2.9.3 >> >> _______________________________________________ >> ModemManager-devel mailing list >> <a href="mailto:ModemManager-devel@lists.freedesktop.org">ModemManager-devel@lists.freedesktop.org</a> >> <a href="https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel">https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel</a> > > > > -- > Aleksander > <a href="https://aleksander.es">https://aleksander.es</a> >