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

[Aleksander Morgado]

On 30/11/13 22:19, Ben Chan wrote:
> +    MbimMessage *response;
> +    GError *error = NULL;
> +    MbimProvider *provider;
> +
> +    response = mbim_device_command_finish (device, res, &error);
> +    if (response &&
> +        mbim_message_command_done_get_result (response, &error) &&
> +        mbim_message_home_provider_response_parse (
> +            response,
> +            &provider,
> +            &error)) {
> +        g_simple_async_result_set_op_res_gpointer (simple, g_strdup (provider->provider_id), NULL);
> +        mbim_provider_free (provider);
> +    } else
> +        g_simple_async_result_take_error (simple, error);
> +
> +    if (response)
> +        mbim_message_unref (response);
> +    g_simple_async_result_complete (simple);
> +    g_object_unref (simple);


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.

-- 
Aleksander