HI Aleksander, 2017-06-28 18:44 GMT+02:00 Aleksander Morgado <aleksan...@aleksander.es>: > Hey Daniele, > > See comments below. > > On 28/06/17 16:07, Daniele Palmas wrote: >> --- >> plugins/telit/mm-common-telit.c | 64 >> +++++++++++++++++++---------------------- >> 1 file changed, 29 insertions(+), 35 deletions(-) >> >> diff --git a/plugins/telit/mm-common-telit.c >> b/plugins/telit/mm-common-telit.c >> index 1e43588..cfd3292 100644 >> --- a/plugins/telit/mm-common-telit.c >> +++ b/plugins/telit/mm-common-telit.c >> @@ -120,10 +120,7 @@ out: >> /* Custom init */ >> >> typedef struct { >> - MMPortProbe *probe; >> MMPortSerialAt *port; >> - GCancellable *cancellable; >> - GSimpleAsyncResult *result; >> gboolean getportcfg_done; >> guint getportcfg_retries; >> } TelitCustomInitContext; >> @@ -133,10 +130,10 @@ telit_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 telit_custom_init_step (TelitCustomInitContext *ctx); >> +static void telit_custom_init_step (GTask *task); >> >> static gboolean >> cache_port_mode (MMDevice *device, >> @@ -217,10 +214,15 @@ out: >> static void >> getportcfg_ready (MMPortSerialAt *port, >> GAsyncResult *res, >> - TelitCustomInitContext *ctx) >> + GTask *task) >> { >> const gchar *response; >> GError *error = NULL; >> + MMPortProbe *probe; >> + TelitCustomInitContext *ctx; >> + >> + ctx = g_task_get_task_data (task); >> + probe = g_task_get_source_object (task); >> >> response = mm_port_serial_at_command_finish (port, res, &error); >> if (error) { >> @@ -237,7 +239,7 @@ getportcfg_ready (MMPortSerialAt *port, >> } else { >> MMDevice *device; >> >> - device = mm_port_probe_peek_device (ctx->probe); >> + device = mm_port_probe_peek_device (probe); >> >> /* Results are cached in the parent device object */ >> if (g_object_get_data (G_OBJECT (device), TAG_GETPORTCFG_SUPPORTED) >> == NULL) { >> @@ -249,35 +251,34 @@ getportcfg_ready (MMPortSerialAt *port, >> } >> >> /* Port answered to #PORTCFG, so mark it as being AT already */ >> - mm_port_probe_set_result_at (ctx->probe, TRUE); >> + mm_port_probe_set_result_at (probe, TRUE); >> } >> >> if (error) >> g_error_free (error); >> >> - telit_custom_init_step (ctx); >> + telit_custom_init_step (task); >> } >> >> static void >> -telit_custom_init_context_complete_and_free (TelitCustomInitContext *ctx) >> +telit_custom_init_context_free (TelitCustomInitContext *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 (TelitCustomInitContext, ctx); >> } >> >> static void >> -telit_custom_init_step (TelitCustomInitContext *ctx) >> +telit_custom_init_step (GTask *task) >> { >> MMKernelDevice *port; >> + MMPortProbe *probe; >> + TelitCustomInitContext *ctx; >> + >> + ctx = g_task_get_task_data (task); >> + probe = g_task_get_source_object (task); >> >> /* If cancelled, end */ >> - if (g_cancellable_is_cancelled (ctx->cancellable)) { >> + if (g_task_return_error_if_cancelled (task)) { >> mm_dbg ("telit: no need to keep on running custom init in (%s)", >> mm_port_get_device (MM_PORT (ctx->port))); >> goto out; > > This is not right. The g_task_return_error_if_cancelled() already does the > GTask return, so you cannot "goto out" and try to g_task_return_boolean() > again there. In this specific case we do want to return TRUE, so we should be > checking manually the cancellable ourselves (i.e. without returning the > GTask), with: > if (g_cancellable_is_cancelled (g_task_get_cancellable ())) { > ... > } > > AND also, as we want the finish() method to return TRUE, we should also > g_task_set_check_cancellable (task, FALSE) as soon as we create it. > Otherwise, even if we return TRUE with g_task_return_boolean() the GTask > would end up returning a cancellable error instead. >
Thanks for the review, I will send a new version of the patch. Regards, Daniele > >> @@ -286,7 +287,7 @@ telit_custom_init_step (TelitCustomInitContext *ctx) >> /* Try to get a port configuration from the modem: usb interface 00 >> * is always linked to an AT port >> */ >> - port = mm_port_probe_peek_port (ctx->probe); >> + port = mm_port_probe_peek_port (probe); >> if (!ctx->getportcfg_done && >> g_strcmp0 (mm_kernel_device_get_property (port, >> "ID_USB_INTERFACE_NUM"), "00") == 0) { >> >> @@ -300,15 +301,15 @@ telit_custom_init_step (TelitCustomInitContext *ctx) >> 2, >> FALSE, /* raw */ >> FALSE, /* allow_cached */ >> - ctx->cancellable, >> + g_task_get_cancellable (task), >> (GAsyncReadyCallback)getportcfg_ready, >> - ctx); >> + task); >> return; >> } >> >> out: >> - g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); >> - telit_custom_init_context_complete_and_free (ctx); >> + g_task_return_boolean (task, TRUE); >> + g_object_unref (task); >> } >> >> void >> @@ -318,23 +319,16 @@ telit_custom_init (MMPortProbe *probe, >> GAsyncReadyCallback callback, >> gpointer user_data) >> { >> - MMDevice *device; >> - MMKernelDevice *port_device; >> TelitCustomInitContext *ctx; >> - >> - device = mm_port_probe_peek_device (probe); >> - port_device = mm_port_probe_peek_port (probe); >> + GTask *task; >> >> ctx = g_slice_new (TelitCustomInitContext); >> - ctx->result = g_simple_async_result_new (G_OBJECT (probe), >> - callback, >> - user_data, >> - telit_custom_init); >> - ctx->probe = g_object_ref (probe); >> ctx->port = g_object_ref (port); >> - ctx->cancellable = cancellable ? g_object_ref (cancellable) : NULL; >> ctx->getportcfg_done = FALSE; >> ctx->getportcfg_retries = 3; >> >> - telit_custom_init_step (ctx); >> + task = g_task_new (probe, cancellable, callback, user_data); >> + g_task_set_task_data (task, ctx, >> (GDestroyNotify)telit_custom_init_context_free); >> + >> + telit_custom_init_step (task); >> } >> > > > -- > Aleksander > https://aleksander.es _______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel