[PATCH v2] atmodem/atutil.h: cb_data ref/unref possibility

[PATCH v2] atmodem/atutil.h: cb_data ref/unref possibility

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 3093 bytes --]

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.

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;
+
+	g_atomic_int_inc(&cbd->ref_count);
+
+	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;
-- 
2.17.1

Re: [PATCH v2] atmodem/atutil.h: cb_data ref/unref possibility

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4146 bytes --]

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.  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?

> 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.

Regards,
-Denis

Re: [PATCH v2] atmodem/atutil.h: cb_data ref/unref possibility

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 5187 bytes --]

Hi Denis,


On Thu, Oct 25, 2018 at 6:13 PM Denis Kenzior <denkenz@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