Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem-qmi
Hey, > > On Thu, May 10, 2018 at 3:47 PM, Ben Chan wrote: >> This branch contains a series of patches that port the remaining code in >> MMBroadbandQmi to use GTask: >> >> https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:gtask-broadband-modem-qmi > > Gave it a quick look and it looks good to me. > Let's schedule to merge this after we've released 1.8. I think we > should also include these changes in the MM 1.8.x stable branch once > it's created, so that we can target some 1.8.x release fully ported to > GTask. > This is now in git master (not released in 1.8). Will cherry-pick them for the 1.8.x branch once they have been tested some time. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem-qmi
Hey, On Thu, May 10, 2018 at 3:47 PM, Ben Chan wrote: > This branch contains a series of patches that port the remaining code in > MMBroadbandQmi to use GTask: > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:gtask-broadband-modem-qmi Gave it a quick look and it looks good to me. Let's schedule to merge this after we've released 1.8. I think we should also include these changes in the MM 1.8.x stable branch once it's created, so that we can target some 1.8.x release fully ported to GTask. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem
On Mon, Jul 17, 2017 at 5:42 PM, Ben Chan wrote: >> >> Please rebase your branch on top of master, include the missing >> commits you took out, and repush for review. > > Done. Merged the branch to git master now, thanks! -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem
On Mon, Jul 17, 2017 at 1:25 AM, Aleksander Morgado wrote: > On Mon, Jul 17, 2017 at 10:02 AM, Aleksander Morgado > wrote: >>> What you included here as a >>> snippet is a huge bug actually. >> >> Well, not that huge really... the only issue is that we're setting the >> GSimpleAsyncResult result twice to TRUE. But, of course, if the >> parent's implementation isn't using a GSimpleAsyncResult and is using >> a GTask instead (as in your case), there really would be a bug as a >> GTask isn't a GSimpleAsyncResult :) So, let me fix as many of those >> issues as I find in a quick look, and then publish your full branch >> and we'll get it reviewed and merged. > > I did the following grep in the source code to find all places where > we were casting explicitly to G_SIMPLE_ASYNC_RESULT when calling > g_simple_async_result_set_op...(): > > $ grep -r "g_simple_async_result_set_op" . | grep "G_SIMPLE_ASYNC" > > And fixed all places reported. Thanks for the quick fix. > > The rationale is that we usually explicitly use the GSimpleAsyncResult > type instead of "gpointer" in the user_data passed to the async > functions inside the async function we're implementing, so there > usually is never the need to explicitly cast that to > GSimpleAsyncResult again to call g_simple_async_result_set_op...(). > Where there was an explicit cast, the code was buggy. > > Please rebase your branch on top of master, include the missing > commits you took out, and repush for review. Done. > > -- > Aleksander > https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem
On Mon, Jul 17, 2017 at 10:02 AM, Aleksander Morgado wrote: >> What you included here as a >> snippet is a huge bug actually. > > Well, not that huge really... the only issue is that we're setting the > GSimpleAsyncResult result twice to TRUE. But, of course, if the > parent's implementation isn't using a GSimpleAsyncResult and is using > a GTask instead (as in your case), there really would be a bug as a > GTask isn't a GSimpleAsyncResult :) So, let me fix as many of those > issues as I find in a quick look, and then publish your full branch > and we'll get it reviewed and merged. I did the following grep in the source code to find all places where we were casting explicitly to G_SIMPLE_ASYNC_RESULT when calling g_simple_async_result_set_op...(): $ grep -r "g_simple_async_result_set_op" . | grep "G_SIMPLE_ASYNC" And fixed all places reported. The rationale is that we usually explicitly use the GSimpleAsyncResult type instead of "gpointer" in the user_data passed to the async functions inside the async function we're implementing, so there usually is never the need to explicitly cast that to GSimpleAsyncResult again to call g_simple_async_result_set_op...(). Where there was an explicit cast, the code was buggy. Please rebase your branch on top of master, include the missing commits you took out, and repush for review. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem
On Mon, Jul 17, 2017 at 9:52 AM, Aleksander Morgado wrote: > What you included here as a > snippet is a huge bug actually. Well, not that huge really... the only issue is that we're setting the GSimpleAsyncResult result twice to TRUE. But, of course, if the parent's implementation isn't using a GSimpleAsyncResult and is using a GTask instead (as in your case), there really would be a bug as a GTask isn't a GSimpleAsyncResult :) So, let me fix as many of those issues as I find in a quick look, and then publish your full branch and we'll get it reviewed and merged. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem
On Mon, Jul 17, 2017 at 8:49 AM, Ben Chan 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
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem
On Sun, Jul 16, 2017 at 11:03 PM, Ben Chan wrote: > This branch contains a series of patches that port > MMBroadbandModemMbim to use GTask: > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:gtask-broadband-modem Aleksander, 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. > > Ben ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem-mbim
On 11/07/17 19:38, Ben Chan wrote: > This branch contains a series of patches that port MMBroadbandModemMbim to > use GTask: > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:gtask-broadband-modem-mbim > Pushed to git master, thanks. I also included a follow up commit to remove an unused variable. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem-mbim
On 11/07/17 19:38, Ben Chan wrote: > This branch contains a series of patches that port MMBroadbandModemMbim to > use GTask: > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:gtask-broadband-modem-mbim > Pushed to git master, thanks. I also included a follow up commit to remove an unused variable. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
