[PATCH] broadband-modem-huawei: implement Modem.Signal extended signal info interface

[Aleksander Morgado]

On Mon, Aug 29, 2016 at 5:33 PM, Dan Williams <dcbw at redhat.com> wrote:
>> > +static gboolean
>> > +signal_load_values_finish (MMIfaceModemSignal *self,
>> > +                           GAsyncResult *res,
>> > +                           MMSignal **cdma,
>> > +                           MMSignal **evdo,
>> > +                           MMSignal **gsm,
>> > +                           MMSignal **umts,
>> > +                           MMSignal **lte,
>> > +                           GError **error)
>> > +{
>> > +    DetailedSignal *values_result;
>> > +
>> > +    values_result = g_task_propagate_pointer (G_TASK (res),
>> > error);
>> > +    if (!values_result)
>> > +        return FALSE;
>> > +
>> > +    /* References from hcsq_get_ready() transferred to caller so
>> > we don't
>> > +     * need to unref them elsewhere.
>> > +     */
>> > +    *cdma = values_result->cdma;
>> > +    *evdo = values_result->evdo;
>> > +    *gsm  = values_result->gsm;
>> > +    *umts = values_result->umts;
>> > +    *lte  = values_result->lte;
>> "values_result" leaks here.
>
> Actually it shouldn't due to the magic of GTask, because:
>
>> >
>> > +    values_result = g_slice_new0 (DetailedSignal);
>> > +    values_result->cdma = self->priv->detailed_signal.cdma ?
>> > g_object_ref (self->priv->detailed_signal.cdma) : NULL;
>> > +    values_result->evdo = self->priv->detailed_signal.evdo ?
>> > g_object_ref (self->priv->detailed_signal.evdo) : NULL;
>> > +    values_result->gsm = self->priv->detailed_signal.gsm ?
>> > g_object_ref (self->priv->detailed_signal.gsm) : NULL;
>> > +    values_result->umts = self->priv->detailed_signal.umts ?
>> > g_object_ref (self->priv->detailed_signal.umts) : NULL;
>> > +    values_result->lte = self->priv->detailed_signal.lte ?
>> > g_object_ref (self->priv->detailed_signal.lte) : NULL;
>> > +
>> > +    g_task_return_pointer (task, values_result,
>> > (GDestroyNotify)detailed_signal_free);
>> > +    g_object_unref (task);
>
> The detailed_signal_free() on the task pointer should free
> values_result when the GTask is destroyed.  This is the first GTask
> conversion I've done though, so does that look like it handles the
> cleanup correctly?

The g_task_return_pointer() transfers full ownership (transfer-full)
of the pointer that was set with g_task_return_pointer(). In this
case, the DetailedSignal struct will be returned with full ownership,
so an explicit detailed_signal_free() is needed when no longer used.

The GDestroyNotify that you pass in g_task_return_pointer() is only
executed if the task is cancelled (e.g. between
g_task_return_pointer() and the actual g_task_propagate_pointer()), so
that the GTask knows how to free the data and return NULL with a
G_IO_ERROR_CANCELLED at the same time.

-- 
Aleksander
https://aleksander.es