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

[Denis Kenzior]

Hi Giacinto,

the cb_data can be used by creating the structure with cb_data_new,
- use it in a single callback function, and destroy it with a call to
g_free.
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);
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.
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);
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);
}
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