<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Hi Aleksander,</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">I tested this patch and it looks good to me (I specially like the second one). There is just a little mistake I made in my part of the patch. In the "status changed" log I inverted current status with the previous one.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default"><div class="gmail_default"><font face="arial, helvetica, sans-serif">---</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> plugins/telit/mm-broadband-modem-telit.c | 6 +++---</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> 1 file changed, 3 insertions(+), 3 deletions(-)</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"><br></font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">index 7c93c0d..e78a195 100644</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">--- a/plugins/telit/mm-broadband-modem-telit.c</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+++ b/plugins/telit/mm-broadband-modem-telit.c</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">@@ -125,9 +125,9 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">     self->priv->qss_status = cur_qss_status;</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> </font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">     if (cur_qss_status != prev_qss_status)</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">-        mm_dbg ("QSS: status changed '%s -> %s",</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">-                mm_telit_qss_status_get_string (cur_qss_status),</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">-                mm_telit_qss_status_get_string (prev_qss_status));</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+        mm_dbg ("QSS: status changed %s -> %s",</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+                mm_telit_qss_status_get_string (prev_qss_status),</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+                mm_telit_qss_status_get_string (cur_qss_status));</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> </font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">     if ((prev_qss_status == QSS_STATUS_SIM_REMOVED && cur_qss_status != QSS_STATUS_SIM_REMOVED) ||</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">         (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status == QSS_STATUS_SIM_REMOVED)) {</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">--</font></div></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Can I submit a little patch just for that or do you prefer doing otherwise?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 2 June 2017 at 13:10, Aleksander Morgado <span dir="ltr"><<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">When the async method starts we store already the primary and the<br>
optional secondary port objects in the method context, keeping a full<br>
reference for each.<br>
<br>
When we parse the response for the enabling command, we just reuse the<br>
stored port objects, instead of re-querying the modem to get them, and<br>
this makes sure that the port objects where we want to set the<br>
unsolicited message handlers are still valid. If the ports are gone<br>
in the middle of the enabling operation, the handlers will be set<br>
without errors, even if the ports may likely get completely disposed<br>
when this async method context is disposed.<br>
<br>
Additionally, we make sure that we return an error also for the case<br>
where there is no secondary port and the enabling on the primary port<br>
failed.<br>
<br>
This patch also fixes the use of the at_command_full_finish() method to<br>
complete the at_command_full() async operation, to keep consistency.<br>
---<br>
 plugins/telit/mm-broadband-<wbr>modem-telit.c | 102 +++++++++++++-----------------<wbr>-<br>
 1 file changed, 41 insertions(+), 61 deletions(-)<br>
<br>
diff --git a/plugins/telit/mm-broadband-<wbr>modem-telit.c b/plugins/telit/mm-broadband-<wbr>modem-telit.c<br>
index 2c1c5420..e609bab1 100644<br>
--- a/plugins/telit/mm-broadband-<wbr>modem-telit.c<br>
+++ b/plugins/telit/mm-broadband-<wbr>modem-telit.c<br>
@@ -103,6 +103,8 @@ typedef enum {<br>
<br>
 typedef struct {<br>
     QssSetupStep step;<br>
+    MMPortSerialAt *primary;<br>
+    MMPortSerialAt *secondary;<br>
     GError *primary_error;<br>
     GError *secondary_error;<br>
 } QssSetupContext;<br>
@@ -138,6 +140,8 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,<br>
 static void<br>
 qss_setup_context_free (QssSetupContext *ctx)<br>
 {<br>
+    g_clear_object (&(ctx->primary));<br>
+    g_clear_object (&(ctx->secondary));<br>
     g_clear_error (&(ctx->primary_error));<br>
     g_clear_error (&(ctx->secondary_error));<br>
     g_slice_free (QssSetupContext, ctx);<br>
@@ -156,60 +160,37 @@ telit_qss_enable_ready (MMBaseModem *modem,<br>
                         GAsyncResult *res,<br>
                         GTask *task)<br>
 {<br>
-    GError *error = NULL;<br>
     MMBroadbandModemTelit *self;<br>
     QssSetupContext *ctx;<br>
-    MMPortSerialAt *primary;<br>
-    MMPortSerialAt *secondary;<br>
+    MMPortSerialAt *port;<br>
+    GError **error;<br>
     GRegex *pattern;<br>
<br>
     self = MM_BROADBAND_MODEM_TELIT (g_task_get_source_object (task));<br>
     ctx = g_task_get_task_data (task);<br>
<br>
-    mm_base_modem_at_command_<wbr>finish (modem, res, &error);<br>
-    if (error) {<br>
-        if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT) {<br>
-            mm_warn ("QSS: error enabling unsolicited on primary port: %s", error->message);<br>
-            ctx->primary_error = error;<br>
-        } else if (ctx->step == QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT) {<br>
-            mm_warn ("QSS: error enabling unsolicited on secondary port: %s", error->message);<br>
-            ctx->secondary_error = error;<br>
-        } else {<br>
-            g_assert_not_reached ();<br>
-        }<br>
+    if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT) {<br>
+        port = ctx->primary;<br>
+        error = &ctx->primary_error;<br>
+    } else if (ctx->step == QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT) {<br>
+        port = ctx->secondary;<br>
+        error = &ctx->secondary_error;<br>
+    } else<br>
+        g_assert_not_reached ();<br>
+<br>
+    if (!mm_base_modem_at_command_<wbr>full_finish (modem, res, error)) {<br>
+        mm_warn ("QSS: error enabling unsolicited on port %s: %s", mm_port_get_device (MM_PORT (port)), (*error)->message);<br>
         goto next_step;<br>
     }<br>
<br>
     pattern = g_regex_new ("#QSS:\\s*([0-3])\\r\\n", G_REGEX_RAW, 0, NULL);<br>
     g_assert (pattern);<br>
-<br>
-    if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT) {<br>
-        primary = mm_base_modem_peek_port_<wbr>primary (MM_BASE_MODEM (self));<br>
-        mm_port_serial_at_add_<wbr>unsolicited_msg_handler (<br>
-            primary,<br>
-            pattern,<br>
-            (<wbr>MMPortSerialAtUnsolicitedMsgFn<wbr>)telit_qss_unsolicited_<wbr>handler,<br>
-            self,<br>
-            NULL);<br>
-    }<br>
-<br>
-    if (ctx->step == QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT) {<br>
-        secondary = mm_base_modem_peek_port_<wbr>secondary (MM_BASE_MODEM (self));<br>
-        if (!secondary) {<br>
-            mm_port_serial_at_add_<wbr>unsolicited_msg_handler (<br>
-                secondary,<br>
-                pattern,<br>
-                (<wbr>MMPortSerialAtUnsolicitedMsgFn<wbr>)telit_qss_unsolicited_<wbr>handler,<br>
-                self,<br>
-                NULL);<br>
-        } else {<br>
-            mm_warn ("QSS could not set handler on secondary port: no secondary port found.");<br>
-            ctx->secondary_error = g_error_new (MM_CORE_ERROR,<br>
-                                                MM_CORE_ERROR_FAILED,<br>
-                                                "QSS could not set handler hat secondary port: no secondary port found.");<br>
-        }<br>
-    }<br>
-<br>
+    mm_port_serial_at_add_<wbr>unsolicited_msg_handler (<br>
+        port,<br>
+        pattern,<br>
+        (<wbr>MMPortSerialAtUnsolicitedMsgFn<wbr>)telit_qss_unsolicited_<wbr>handler,<br>
+        self,<br>
+        NULL);<br>
     g_regex_unref (pattern);<br>
<br>
 next_step:<br>
@@ -276,7 +257,7 @@ qss_setup_step (GTask *task)<br>
             return;<br>
         case QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT:<br>
             mm_base_modem_at_command_full (MM_BASE_MODEM (self),<br>
-                                           mm_base_modem_peek_port_<wbr>primary (MM_BASE_MODEM (self)),<br>
+                                           ctx->primary,<br>
                                            "#QSS=1",<br>
                                            3,<br>
                                            FALSE,<br>
@@ -285,35 +266,32 @@ qss_setup_step (GTask *task)<br>
                                            (GAsyncReadyCallback) telit_qss_enable_ready,<br>
                                            task);<br>
             return;<br>
-        case QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT: {<br>
-            MMPortSerialAt *port;<br>
-<br>
-            port = mm_base_modem_peek_port_<wbr>secondary (MM_BASE_MODEM (self));<br>
-            if (port) {<br>
+        case QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT:<br>
+            if (ctx->secondary) {<br>
                 mm_base_modem_at_command_full (MM_BASE_MODEM (self),<br>
-                                           port,<br>
-                                           "#QSS=1",<br>
-                                           3,<br>
-                                           FALSE,<br>
-                                           FALSE, /* raw */<br>
-                                           NULL, /* cancellable */<br>
-                                           (GAsyncReadyCallback) telit_qss_enable_ready,<br>
-                                           task);<br>
+                                               ctx->secondary,<br>
+                                               "#QSS=1",<br>
+                                               3,<br>
+                                               FALSE,<br>
+                                               FALSE, /* raw */<br>
+                                               NULL, /* cancellable */<br>
+                                               (GAsyncReadyCallback) telit_qss_enable_ready,<br>
+                                               task);<br>
                 return;<br>
             }<br>
-<br>
             /* Fall back to next step */<br>
             ctx->step++;<br>
-        }<br>
         case QSS_SETUP_STEP_LAST:<br>
-            if (ctx->primary_error && ctx->secondary_error) {<br>
+            /* If all enabling actions failed (either both, or only primary if<br>
+             * there is no secondary), then we return an error */<br>
+            if (ctx->primary_error &&<br>
+                (ctx->secondary_error || !ctx->secondary))<br>
                 g_task_return_new_error (task,<br>
                                          MM_CORE_ERROR,<br>
                                          MM_CORE_ERROR_FAILED,<br>
                                          "QSS: couldn't enable unsolicited");<br>
-            } else {<br>
+            else<br>
                 g_task_return_boolean (task, TRUE);<br>
-            }<br>
             g_object_unref (task);<br>
             break;<br>
     }<br>
@@ -331,6 +309,8 @@ modem_setup_sim_hot_swap (MMIfaceModem *self,<br>
<br>
     ctx = g_slice_new0 (QssSetupContext);<br>
     ctx->step = QSS_SETUP_STEP_FIRST;<br>
+    ctx->primary = mm_base_modem_get_port_primary (MM_BASE_MODEM (self));<br>
+    ctx->secondary = mm_base_modem_get_port_<wbr>secondary (MM_BASE_MODEM (self));<br>
<br>
     g_task_set_task_data (task, ctx, (GDestroyNotify) qss_setup_context_free);<br>
     qss_setup_step (task);<br>
<span class="HOEnZb"><font color="#888888">--<br>
2.13.0<br>
<br>
</font></span></blockquote></div><br></div>