sgtm. Forgot to add --in-reply-to, so the revised patches are sent to:
https://lists.freedesktop.org/archives/modemmanager-devel/2017-April/004394.html
https://lists.freedesktop.org/archives/modemmanager-devel/2017-April/004395.html
On Sun, Apr 2, 2017 at 12:28 AM, Aleksander Morgado
<aleksan...@aleksander.es> wrote:
>
> Hey Ben,
>
> Looks like these 2 things could really be split into 2 different
> patches, could you do that?
>
> See other comments below inline.
>
> On Sat, Apr 1, 2017 at 8:05 PM, Ben Chan <benc...@chromium.org> wrote:
> > ---
> > plugins/novatel/mm-common-novatel.c | 54 +++++++++++-----------
> > plugins/novatel/mm-sim-novatel-lte.c | 89
> > ++++++++++++++++--------------------
> > 2 files changed, 66 insertions(+), 77 deletions(-)
> >
> > diff --git a/plugins/novatel/mm-common-novatel.c
> > b/plugins/novatel/mm-common-novatel.c
> > index 4c39c7c7..32a90a66 100644
> > --- a/plugins/novatel/mm-common-novatel.c
> > +++ b/plugins/novatel/mm-common-novatel.c
> > @@ -23,21 +23,17 @@ typedef struct {
> > MMPortProbe *probe;
> > MMPortSerialAt *port;
> > GCancellable *cancellable;
>
> You wouldn't really require a GCancellable in the context any more;
> just pass it to the GTask.
>
> > - GSimpleAsyncResult *result;
> > guint nwdmat_retries;
> > guint wait_time;
> > } CustomInitContext;
> >
> > static void
> > -custom_init_context_complete_and_free (CustomInitContext *ctx)
> > +custom_init_context_free (CustomInitContext *ctx)
> > {
> > - g_simple_async_result_complete_in_idle (ctx->result);
> > -
> > if (ctx->cancellable)
> > g_object_unref (ctx->cancellable);
> > g_object_unref (ctx->port);
> > g_object_unref (ctx->probe);
> > - g_object_unref (ctx->result);
> > g_slice_free (CustomInitContext, ctx);
> > }
> >
> > @@ -46,15 +42,15 @@ mm_common_novatel_custom_init_finish (MMPortProbe
> > *probe,
> > GAsyncResult *result,
> > GError **error)
> > {
> > - return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT
> > (result), error);
> > + return g_task_propagate_boolean (G_TASK (result), error);
> > }
> >
> > -static void custom_init_step (CustomInitContext *ctx);
> > +static void custom_init_step (GTask *task);
> >
> > static void
> > nwdmat_ready (MMPortSerialAt *port,
> > GAsyncResult *res,
> > - CustomInitContext *ctx)
> > + GTask* task)
> > {
> > const gchar *response;
> > GError *error = NULL;
> > @@ -64,7 +60,7 @@ nwdmat_ready (MMPortSerialAt *port,
> > if (g_error_matches (error,
> > MM_SERIAL_ERROR,
> > MM_SERIAL_ERROR_RESPONSE_TIMEOUT)) {
> > - custom_init_step (ctx);
> > + custom_init_step (task);
> > goto out;
> > }
> >
> > @@ -72,8 +68,8 @@ nwdmat_ready (MMPortSerialAt *port,
> > }
> >
> > /* Finish custom_init */
> > - g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> > - custom_init_context_complete_and_free (ctx);
> > + g_task_return_boolean (task, TRUE);
> > + g_object_unref (task);
> >
> > out:
> > if (error)
> > @@ -81,21 +77,25 @@ out:
> > }
> >
> > static gboolean
> > -custom_init_wait_cb (CustomInitContext *ctx)
> > +custom_init_wait_cb (GTask *task)
> > {
> > - custom_init_step (ctx);
> > + custom_init_step (task);
> > return G_SOURCE_REMOVE;
> > }
> >
> > static void
> > -custom_init_step (CustomInitContext *ctx)
> > +custom_init_step (GTask *task)
> > {
> > + CustomInitContext *ctx;
> > +
> > + ctx = g_task_get_task_data (task);
> > +
> > /* If cancelled, end */
> > if (g_cancellable_is_cancelled (ctx->cancellable)) {
>
> And here you would g_cancellable_is_cancelled (g_task_get_cancellable (task))
>
> > mm_dbg ("(Novatel) no need to keep on running custom init in (%s)",
> > mm_port_get_device (MM_PORT (ctx->port)));
> > - g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> > - custom_init_context_complete_and_free (ctx);
> > + g_task_return_boolean (task, TRUE);
> > + g_object_unref (task);
> > return;
> > }
> >
> > @@ -103,14 +103,14 @@ custom_init_step (CustomInitContext *ctx)
> > if (mm_port_probe_list_has_qmi_port (mm_device_peek_port_probe_list
> > (mm_port_probe_peek_device (ctx->probe)))) {
> > mm_dbg ("(Novatel) no need to run custom init in (%s): device has
> > QMI port",
> > mm_port_get_device (MM_PORT (ctx->port)));
> > - g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> > - custom_init_context_complete_and_free (ctx);
> > + g_task_return_boolean (task, TRUE);
> > + g_object_unref (task);
> > return;
> > }
> >
> > if (ctx->wait_time > 0) {
> > ctx->wait_time--;
> > - g_timeout_add_seconds (1, (GSourceFunc)custom_init_wait_cb, ctx);
> > + g_timeout_add_seconds (1, (GSourceFunc)custom_init_wait_cb, task);
> > return;
> > }
> >
> > @@ -123,15 +123,15 @@ custom_init_step (CustomInitContext *ctx)
> > FALSE, /* allow_cached */
> > ctx->cancellable,
> > (GAsyncReadyCallback)nwdmat_ready,
> > - ctx);
> > + task);
> > return;
> > }
> >
> > /* Finish custom_init */
> > mm_dbg ("(Novatel) couldn't flip secondary port to AT in (%s): all
> > retries consumed",
> > mm_port_get_device (MM_PORT (ctx->port)));
> > - g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> > - custom_init_context_complete_and_free (ctx);
> > + g_task_return_boolean (task, TRUE);
> > + g_object_unref (task);
> > }
> >
> > void
> > @@ -142,17 +142,17 @@ mm_common_novatel_custom_init (MMPortProbe *probe,
> > gpointer user_data)
> > {
> > CustomInitContext *ctx;
> > + GTask *task;
> >
> > ctx = g_slice_new (CustomInitContext);
> > - ctx->result = g_simple_async_result_new (G_OBJECT (probe),
> > - callback,
> > - user_data,
> > -
> > mm_common_novatel_custom_init);
> > ctx->probe = g_object_ref (probe);
> > ctx->port = g_object_ref (port);
> > ctx->cancellable = cancellable ? g_object_ref (cancellable) : NULL;
> > ctx->nwdmat_retries = 3;
> > ctx->wait_time = 2;
> >
> > - custom_init_step (ctx);
> > + task = g_task_new (probe, NULL, callback, user_data);
>
> And here you would just pass cancellable instead of NULL.
>
> > + g_task_set_task_data (task, ctx,
> > (GDestroyNotify)custom_init_context_free);
> > +
> > + custom_init_step (task);
> > }
> > diff --git a/plugins/novatel/mm-sim-novatel-lte.c
> > b/plugins/novatel/mm-sim-novatel-lte.c
> > index 41d30411..3ec1671b 100644
> > --- a/plugins/novatel/mm-sim-novatel-lte.c
> > +++ b/plugins/novatel/mm-sim-novatel-lte.c
> > @@ -41,18 +41,17 @@ load_imsi_finish (MMBaseSim *self,
> > {
> > gchar *imsi;
> >
> > - if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT
> > (res), error))
> > - return NULL;
> > + imsi = g_task_propagate_pointer (G_TASK (res), error);
> > + if (imsi)
> > + mm_dbg ("loaded IMSI: %s", imsi);
> >
> > - imsi = g_simple_async_result_get_op_res_gpointer
> > (G_SIMPLE_ASYNC_RESULT (res));
> > - mm_dbg ("loaded IMSI: %s", imsi);
> > - return g_strdup (imsi);
> > + return imsi;
>
> I'm tempted to say that the mm_dbg () isn't of much use, and you could just:
> return g_task_propagate_pointer (G_TASK (res), error);
>
> > }
> >
> > static void
> > imsi_read_ready (MMBaseModem *modem,
> > GAsyncResult *res,
> > - GSimpleAsyncResult *simple)
> > + GTask *task)
> > {
> > GError *error = NULL;
> > const gchar *response, *str;
> > @@ -64,9 +63,8 @@ imsi_read_ready (MMBaseModem *modem,
> >
> > response = mm_base_modem_at_command_finish (modem, res, &error);
> > if (!response) {
> > - g_simple_async_result_take_error (simple, error);
> > - g_simple_async_result_complete (simple);
> > - g_object_unref (simple);
> > + g_task_return_error (task, error);
> > + g_object_unref (task);
> > return;
> > }
> >
> > @@ -76,13 +74,12 @@ imsi_read_ready (MMBaseModem *modem,
> > /* With or without quotes... */
> > if (sscanf (str, "%d,%d,\"%18c\"", &sw1, &sw2, (char *) &buf) != 3 &&
> > sscanf (str, "%d,%d,%18c", &sw1, &sw2, (char *) &buf) != 3) {
> > - g_simple_async_result_set_error (simple,
> > - MM_CORE_ERROR,
> > - MM_CORE_ERROR_FAILED,
> > - "Failed to parse the CRSM
> > response: '%s'",
> > - response);
> > - g_simple_async_result_complete (simple);
> > - g_object_unref (simple);
> > + g_task_return_new_error (task,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Failed to parse the CRSM response: '%s'",
> > + response);
> > + g_object_unref (task);
> > return;
> > }
> >
> > @@ -90,13 +87,12 @@ imsi_read_ready (MMBaseModem *modem,
> > (sw1 != 0x91) &&
> > (sw1 != 0x92) &&
> > (sw1 != 0x9f)) {
> > - g_simple_async_result_set_error (simple,
> > - MM_CORE_ERROR,
> > - MM_CORE_ERROR_FAILED,
> > - "SIM failed to handle CRSM
> > request (sw1 %d sw2 %d)",
> > - sw1, sw2);
> > - g_simple_async_result_complete (simple);
> > - g_object_unref (simple);
> > + g_task_return_new_error (task,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "SIM failed to handle CRSM request (sw1
> > %d sw2 %d)",
> > + sw1, sw2);
> > + g_object_unref (task);
> > return;
> > }
> >
> > @@ -114,25 +110,23 @@ imsi_read_ready (MMBaseModem *modem,
> > }
> >
> > /* Invalid character */
> > - g_simple_async_result_set_error (simple,
> > - MM_CORE_ERROR,
> > - MM_CORE_ERROR_FAILED,
> > - "CRSM IMSI response contained
> > invalid character '%c'",
> > - buf[len]);
> > - g_simple_async_result_complete (simple);
> > - g_object_unref (simple);
> > + g_task_return_new_error (task,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "CRSM IMSI response contained invalid
> > character '%c'",
> > + buf[len]);
> > + g_object_unref (task);
> > return;
> > }
> >
> > /* BCD encoded IMSIs plus the length byte and parity are 18 digits
> > long */
> > if (len != 18) {
> > - g_simple_async_result_set_error (simple,
> > - MM_CORE_ERROR,
> > - MM_CORE_ERROR_FAILED,
> > - "Invalid +CRSM IMSI response size
> > (was %zd, expected 18)",
> > - len);
> > - g_simple_async_result_complete (simple);
> > - g_object_unref (simple);
> > + g_task_return_new_error (task,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Invalid +CRSM IMSI response size (was
> > %zd, expected 18)",
> > + len);
> > + g_object_unref (task);
> > return;
> > }
> >
> > @@ -160,19 +154,17 @@ imsi_read_ready (MMBaseModem *modem,
> > /* Ensure all 'F's, if any, are at the end */
> > for (; i < 15; i++) {
> > if (imsi[i] != 'F') {
> > - g_simple_async_result_set_error (simple,
> > - MM_CORE_ERROR,
> > - MM_CORE_ERROR_FAILED,
> > - "Invalid +CRSM IMSI length
> > (unexpected F)");
> > - g_simple_async_result_complete (simple);
> > - g_object_unref (simple);
> > + g_task_return_new_error (task,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Invalid +CRSM IMSI length
> > (unexpected F)");
> > + g_object_unref (task);
> > return;
> > }
> > }
> >
> > - g_simple_async_result_set_op_res_gpointer (simple, imsi, NULL);
> > - g_simple_async_result_complete (simple);
> > - g_object_unref (simple);
> > + g_task_return_pointer (task, g_strdup (imsi), g_free);
> > + g_object_unref (task);
> > }
> >
> > static void
> > @@ -193,10 +185,7 @@ load_imsi (MMBaseSim *self,
> > 3,
> > FALSE,
> > (GAsyncReadyCallback)imsi_read_ready,
> > - g_simple_async_result_new (G_OBJECT (self),
> > - callback,
> > - user_data,
> > - load_imsi));
> > + g_task_new (self, NULL, callback, user_data));
>
> Unrelated to the patch, but we should really update all async methods
> to allow receiving a GCancellable... someday! :)
>
>
> > g_object_unref (modem);
> > }
> >
> > --
> > 2.12.2.564.g063fe858b8-goog
> >
>
>
>
> --
> Aleksander
> https://aleksander.es
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
