Hi Denis,
On Thu, Oct 25, 2018 at 6:13 PM Denis Kenzior <denk...@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/25/2018 12:46 AM, Giacinto Cifelli wrote:
> > the cb_data can be used by creating the structure with cb_data_new,
> > and then there are two possibilities:
> > - use it in a single callback function, and destroy it with a call to
> > g_free.
> > Example:
> > - calling function:
> > struct cb_data *cbd = cb_data_new(cb, data);
> > if (g_at_chat_send(chat, buf, NULL, at_cgatt_cb, cbd, g_free) > 0)
> > return;
> > g_free(cbd);
> > - called function (here at_cgatt_cb):
> > static void at_cgatt_cb(gboolean ok, GAtResult *result,
> > gpointer user_data)
> > {
> > struct cb_data *cbd = user_data;
> > ofono_gprs_cb_t cb = cbd->cb;
> > struct ofono_error error;
> >
> > decode_at_error(&error,
> > g_at_result_final_response(result));
> >
> > cb(&error, cbd->data);
> > }
> > note the absence of explicit g_free(cbd);
> >
> > - pass it through a train of callback functions, adding a reference at
> > each pass cb_data_ref, and removing it with cb_data_unref.
> > the use of cb_data_ref would replace a new object creation, while the
> > use of cb_data_unref the use of g_free.
> > Example:
> > - calling function:
> > struct cb_data *cbd = cb_data_new(cb, data);
> > // no cb_ref at the creation
> > if (g_at_chat_send(chat, buf, NULL,
> > at_lte_set_default_attach_info_cb,
> > cbd, cb_data_unref) > 0)
> > goto end;
> > cb_data_unref(cbd);
> > - called function 1 (at_lte_set_default_attach_info_cb):
> > static void at_lte_set_default_attach_info_cb(gboolean ok,
> > GAtResult *result, gpointer user_data)
> > {
> > struct cb_data *cbd = user_data;
> >
> > cbd = cb_data_ref(cbd);
> > if (g_at_chat_send(chat, buf, NULL,
> > at_cgatt_cb, cbd, cb_data_unref) > 0)
> > return;
> > cb_data_unref(cbd);
> > }
> > - called function 2 (at_cgatt_cb):
> > like above. no call to g_free or cb_data_unref. The terminal function
> > doesn't need to know about the reference scheme.
> >
>
> This is a nice summary of a very common pattern.
yes, I felt very creative this morning :)
> I left it in the
> commit description, but perhaps you might want to break this out into a
> separate document (and make it even more digestible) so that we can
> point people to it more easily.
>
> Perhaps something like doc/common-patterns.txt?
ok, I will start such a document.
>
> > v2: fixed prototype for cb_data_unref to be compatible with g_free
> > ---
> > drivers/atmodem/atutil.h | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
> > index f1389a94..aa8b619c 100644
> > --- a/drivers/atmodem/atutil.h
> > +++ b/drivers/atmodem/atutil.h
> > @@ -104,6 +104,7 @@ char *at_util_get_cgdcont_command(guint cid, enum
> > ofono_gprs_proto proto,
> > const char *apn);
> >
> > struct cb_data {
> > + gint ref_count;
> > void *cb;
> > void *data;
> > void *user;
> > @@ -114,12 +115,37 @@ static inline struct cb_data *cb_data_new(void *cb,
> > void *data)
> > struct cb_data *ret;
> >
> > ret = g_new0(struct cb_data, 1);
> > + ret->ref_count = 1;
> > ret->cb = cb;
> > ret->data = data;
> >
> > return ret;
> > }
> >
> > +static inline struct cb_data *cb_data_ref(struct cb_data *cbd)
> > +{
> > + if (cbd == NULL)
> > + return NULL;
>
> I removed this part. For all private/semi-private APIs I want us to
> crash early. If someone is making the mistake of passing a NULL to
> cb_data_ref then I don't want it to remain hidden until the bug
> manifests itself in harder to detect ways.
>
> > +
> > + g_atomic_int_inc(&cbd->ref_count);
>
> And I removed g_atomic stuff as well...
>
> > +
> > + return cbd;
> > +}
> > +
> > +static inline void cb_data_unref(gpointer user_data)
> > +{
> > + struct cb_data *cbd = user_data;
> > + gboolean is_zero;
> > +
> > + if (cbd == NULL)
> > + return;
> > +
> > + is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
> > +
> > + if (is_zero == TRUE)
> > + g_free(cbd);
> > +}
> > +
> > static inline int at_util_convert_signal_strength(int strength)
> > {
> > int result;
> >
>
> Applied as commit eed785a4d7a2529a5c1ddee8ba20b1c4e2aa7f99. Please
> eyeball and make sure I didn't screw anything up.
It looks ok and executes fine (with the gemaltomodem/lte.c that isn't
yet in the tree).
>
> Regards,
> -Denis
Regards,
Giacinto
_______________________________________________
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
