[PATCH] sim-mbim: implement operator identifier and name loading

[Ben Chan]

On Mon, Dec 9, 2013 at 4:26 AM, Aleksander Morgado <aleksander at lanedo.com>wrote:

> One comment here; when setting the result in a GSimpleAsyncResult, you
>  need to make sure that the ownership is fully managed in every case. In
> the case above you're setting a heap-allocated string as a result,
> giving a NULL GDestroyNotify, because you know that _finish() will take
> that string and return it. But that is not really ok, as _finish() may
> never be called if the caller of the async method didn't provide a
> GAsyncReadyCallback. In other words, if I run:
>
>   load_operator_identifier (sim, NULL, NULL);
>
> I would be leaking that string that was set in the GSimpleAsyncResult.
>
>
> To manage this situation properly, you can either:
>
> 1) Don't set a newly allocated string as result in the
> GSimpleAsyncResult. In your example, you could have done:
>
>   g_simple_async_result_set_op_res_gpointer (simple,
>                                              provider->provider_id,
>                                              NULL);
>   g_simple_async_result_complete (simple);
>   mbim_provider_free (provider);
>
> As you are completing inline (not in idle), the string that you did set
> as result will be valid if _finish() is being called. So then you can
> g_strdup() within _finish(), only if it was ever called.
>
> 2) Set a newly allocated string as result, and provide g_free as
> GDestroyNotify. This will then force you to g_strdup() again during
> _finish(), so you would be allocating twice, but this will make it
> possible to complete_in_idle().
>
> The new GTask API in newer glib manages this logic much better, as it
> allows you to 'steal' the result from the GSimpleAsyncResult (IIRC).
>
>
>
> For this specific case it probably is not a big deal, because I'm not
> sure that we have such calls passing NULL for GAsyncReadyCallback, but
> anyway, I would fix those (e.g. to use approach #1) just for completeness.
>

That makes sense. I'll prepare a patch. I followed load_sim_identifier and
load_imsi. We should probably fix them as well.

Thanks,
Ben
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20131209/e15c74e1/attachment.html>