Skip to contentSkip to conversation

atmodem/atutil.h: cb_data ref/unref possibility

PATCH Applied Giacinto Cifelli 2 participants 3 messages Oct 25, 2018

lore
Share & tools
lore

Giacinto Cifelli

author
#0 lore raw

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

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(+)
drivers/atmodem/atutil.h +26 -0
index f1389a94..aa8b619c 100644
@@ -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;
Signature · 1 line
2.17.1
Applied
#1 lore raw

Hi Giacinto,

Quoted 45 lines · Giacinto Cifelli

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);
Quoted 9 lines
		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?

Quoted 22 lines

v2: fixed prototype for cb_data_unref to be compatible with g_free

---
  drivers/atmodem/atutil.h | 26 ++++++++++++++++++++++++++
  1 file changed, 26 insertions(+)
drivers/atmodem/atutil.h +2 -0
index f1389a94..aa8b619c 100644
@@ -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;

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 128

Quoted 10 lines
drivers/atmodem/atutil.h +4 -0
  	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.

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 130

Quoted 2 lines
drivers/atmodem/atutil.h +2 -0
+
+	g_atomic_int_inc(&cbd->ref_count);

And I removed g_atomic stuff as well...

Quoted 12 lines
drivers/atmodem/atutil.h +11 -0
+
+	return cbd;
+}
+
+static inline void cb_data_unref(gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	gboolean is_zero;
+
+	if (cbd == NULL)
+		return;

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 151

Quoted 10 lines
drivers/atmodem/atutil.h +7 -0
+
+	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

author
#2 lore raw

Hi Denis,

Quoted 59 lines · Denis Kenzior

Hi Giacinto,

Quoted 45 lines · Giacinto Cifelli

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);
Quoted 9 lines
              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.
Quoted 1 line

This is a nice summary of a very common pattern.

yes, I felt very creative this morning :)

Quoted 6 lines

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.

Quoted 61 lines
Quoted 23 lines

v2: fixed prototype for cb_data_unref to be compatible with g_free

---
  drivers/atmodem/atutil.h | 26 ++++++++++++++++++++++++++
  1 file changed, 26 insertions(+)
drivers/atmodem/atutil.h +2 -0
index f1389a94..aa8b619c 100644
@@ -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;

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 128

Quoted 10 lines
drivers/atmodem/atutil.h +4 -0
      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.

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 130

Quoted 2 lines
drivers/atmodem/atutil.h +2 -0
+
+     g_atomic_int_inc(&cbd->ref_count);

And I removed g_atomic stuff as well...

Quoted 17 lines
drivers/atmodem/atutil.h +17 -0
+
+     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);
+}
Quoted 8 lines

drivers/atmodem/atutil.h static inline struct cb_data *cb_data_new(void *cb, void *data) · line 151

Quoted 5 lines
drivers/atmodem/atutil.h +1 -0
+
  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).

Quoted 3 lines

Regards,
-Denis

Regards,
Giacinto