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

[Dan Williams]

On Mon, 2016-08-29 at 20:09 +0200, Aleksander Morgado wrote:
> 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.

Yeah, sorry; I was mixing up the pointer return with
g_task_set_task_data() semantics.  Will fix.

Dan