[PATCH] novatel: port custom init and IMSI loading to use GTask
Aleksander Morgado
aleksander at aleksander.es
Sun Apr 2 07:28:13 UTC 2017
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 <benchan at 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
More information about the ModemManager-devel
mailing list