[review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem

[Aleksander Morgado]

On Mon, Jul 17, 2017 at 8:49 AM, Ben Chan <benchan at chromium.org> wrote:
> Just realized there may be an issue with changes to some of those
> MMIfaceModem3gpp / MMIfaecModemCdma functions implemented by
> MMBroadbandModem. Several plugins have code that chains up parent's
> function like this:
>
>    if (!iface_modem_cdma_parent->cleanup_unsolicited_events_finish
> (self, res, &error))
>            ...
>    else
>            g_simple_async_result_set_op_res_gboolean
> (G_SIMPLE_ASYNC_RESULT (res), TRUE);
>
> which will need some rework. I'll take out those related patches from
> my branch for now.

Wait! Your patches are probably ok. What you included here as a
snippet is a huge bug actually. The "else" branch shouldn't touch the
"res" that was received as input, as that GAsyncResult is already
completed.

The full code (in Huawei plugin) is like this:

static void
parent_cdma_setup_unsolicited_events_ready (MMIfaceModemCdma *self,
                                            GAsyncResult *res,
                                            GSimpleAsyncResult *simple)
{
    GError *error = NULL;

    if (!iface_modem_cdma_parent->setup_unsolicited_events_finish
(self, res, &error))
        g_simple_async_result_take_error (simple, error);
    else {
        /* Our own setup now */
        set_cdma_unsolicited_events_handlers
(MM_BROADBAND_MODEM_HUAWEI (self), TRUE);
        g_simple_async_result_set_op_res_gboolean
(G_SIMPLE_ASYNC_RESULT (res), TRUE);
    }

    g_simple_async_result_complete (simple);
    g_object_unref (simple);
}

It's clear that the g_simple_async_result_set_op_res_gboolean() should
be setting "simple", not "res. Will fix that.

Please update your branch with all the patches that you had,
regardless of issues like this one above.

-- 
Aleksander
https://aleksander.es