Re: [PATCH 1/1] telit: port mm-common-telit to use GTask
HI Aleksander, 2017-06-28 18:44 GMT+02:00 Aleksander Morgado: > 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, ); >> 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
Re: [PATCH 1/1] telit: port mm-common-telit to use GTask
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, ); > 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. > @@ -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),
[PATCH 1/1] telit: port mm-common-telit to use GTask
--- 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, ); 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; @@ -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); -