[PATCH] core: prefer g_task_return_error_if_cancelled() than custom errors

[Ben Chan]

On Mon, Apr 17, 2017 at 9:56 AM, Dan Williams <dcbw at redhat.com> wrote:

> On Sat, 2017-04-15 at 23:26 +0200, Aleksander Morgado wrote:
> > When the GCancellable is added to the GTask, we can use a single
> > method call to check for the task being cancelled, and complete it
> > right away if so.
> >
> > This patch also clears up the logic in the Novatel plugin, where the
> > code was trying to return "TRUE" when the task was cancelled, but
> > wouldn't work as the check-cancellable flag in the GTask is TRUE by
> > default (i.e. when completing the GTask, if it was cancelled, a
> > G_IO_ERROR_CANCELLED would be returned by default, regardless of any
> > other return value set).
> >
> > This patch also introduces a small variation of the logic in the
> > Cinterion plugin: instead of running SWWAN=0 before completing the
> > async action, the command is now sent just after completion of the
> > async action. This shouldn't be an issue, as the SWWAN result itself
> > is ignored.
> >
>
> Patch LGTM.
>
>
lgtm too


> > ---
> >
> > Hey Ben,
> >
> > What do you think of these changes? I believe we should start using
> > this where applicable, instead of the custom checks and errors.
> >
> > ---
> >  plugins/cinterion/mm-broadband-bearer-cinterion.c | 7 ++-----
> >  plugins/novatel/mm-common-novatel.c               | 3 +--
> >  src/mm-auth-provider-polkit.c                     | 6 +-----
> >  src/mm-iface-modem-signal.c                       | 6 +-----
> >  4 files changed, 5 insertions(+), 17 deletions(-)
> >
> > diff --git a/plugins/cinterion/mm-broadband-bearer-cinterion.c
> > b/plugins/cinterion/mm-broadband-bearer-cinterion.c
> > index 741d2935..a1c699e9 100644
> > --- a/plugins/cinterion/mm-broadband-bearer-cinterion.c
> > +++ b/plugins/cinterion/mm-broadband-bearer-cinterion.c
> > @@ -343,10 +343,6 @@ handle_cancel_dial (GTask *task)
> >                                     NULL,
> >                                     NULL);
> >      g_free (command);
> > -
> > -    g_task_return_new_error (task, MM_CORE_ERROR,
> > MM_CORE_ERROR_CANCELLED,
> > -                             "Connection operation has been
> > cancelled");
> > -    g_object_unref (task);
> >  }
> >
> >  static void
> > @@ -357,8 +353,9 @@ dial_3gpp_context_step (GTask *task)
> >      ctx = (Dial3gppContext *) g_task_get_task_data (task);
> >
> >      /* Check for cancellation */
> > -    if (g_cancellable_is_cancelled (g_task_get_cancellable (task)))
> > {
> > +    if (g_task_return_error_if_cancelled (task)) {
> >          handle_cancel_dial (task);
> > +        g_object_unref (task);
> >          return;
> >      }
> >
> > diff --git a/plugins/novatel/mm-common-novatel.c
> > b/plugins/novatel/mm-common-novatel.c
> > index 96d845f1..f8c78f51 100644
> > --- a/plugins/novatel/mm-common-novatel.c
> > +++ b/plugins/novatel/mm-common-novatel.c
> > @@ -87,10 +87,9 @@ custom_init_step (GTask *task)
> >      ctx = g_task_get_task_data (task);
> >
> >      /* If cancelled, end */
> > -    if (g_cancellable_is_cancelled (g_task_get_cancellable (task)))
> > {
> > +    if (g_task_return_error_if_cancelled (task)) {
> >          mm_dbg ("(Novatel) no need to keep on running custom init in
> > (%s)",
> >                  mm_port_get_device (MM_PORT (ctx->port)));
> > -        g_task_return_boolean (task, TRUE);
> >          g_object_unref (task);
> >          return;
> >      }
> > diff --git a/src/mm-auth-provider-polkit.c b/src/mm-auth-provider-
> > polkit.c
> > index 3b9b4250..7e88d67c 100644
> > --- a/src/mm-auth-provider-polkit.c
> > +++ b/src/mm-auth-provider-polkit.c
> > @@ -72,11 +72,7 @@ check_authorization_ready (PolkitAuthority
> > *authority,
> >      GError *error = NULL;
> >      AuthorizeContext *ctx;
> >
> > -    if (g_cancellable_is_cancelled (g_task_get_cancellable (task)))
> > {
> > -        g_task_return_new_error (task,
> > -                                 MM_CORE_ERROR,
> > -                                 MM_CORE_ERROR_CANCELLED,
> > -                                 "PolicyKit authorization attempt
> > cancelled");
> > +    if (g_task_return_error_if_cancelled (task)) {
> >          g_object_unref (task);
> >          return;
> >      }
> > diff --git a/src/mm-iface-modem-signal.c b/src/mm-iface-modem-
> > signal.c
> > index 9751b5d6..99096a95 100644
> > --- a/src/mm-iface-modem-signal.c
> > +++ b/src/mm-iface-modem-signal.c
> > @@ -418,11 +418,7 @@ interface_initialization_step (GTask *task)
> >      InitializationContext *ctx;
> >
> >      /* Don't run new steps if we're cancelled */
> > -    if (g_cancellable_is_cancelled (g_task_get_cancellable (task)))
> > {
> > -        g_task_return_new_error (task,
> > -                                 MM_CORE_ERROR,
> > -                                 MM_CORE_ERROR_CANCELLED,
> > -                                 "Interface initialization
> > cancelled");
> > +    if (g_task_return_error_if_cancelled (task)) {
> >          g_object_unref (task);
> >          return;
> >      }
> > --
> > 2.12.2
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170417/4e4681ab/attachment.html>