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

[Jonas Bonn]

Hi Giacinto,

+static inline struct cb_data *cb_data_ref(struct cb_data *cbd)
+{
+ if (cbd == NULL)
+ return NULL;
+
+ g_atomic_int_inc(&cbd->ref_count);
+

Let me just vent my disagreement with using atomic accessors here again:

i) If I'm a new contributor, unfamiliar with ofono, and I see this then
I think: "oh oh, this is a thread-safe library... better be
careful."... and then I get all confused because nothing else is handled
in a thread-safe manner.

ii) The atomic operations introduce an unnecessary memory barrier.

I _presume_ that the code is written this way because it was inherited
from connman and connman made an early attempt to be thread safe...???
I'm not sure that legacy is worth preserving.

Anyway, enough said on the matter. The implementation is fine aside
from my reservations.

/Jonas

+ return cbd;
+}
+
+static inline void cb_data_unref(struct cb_data *cbd)
+{
+ 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;

[Denis Kenzior]

Hi Jonas,

Post by Jonas Bonn
Hi Giacinto,

+static inline struct cb_data *cb_data_ref(struct cb_data *cbd)
+{
+    if (cbd == NULL)
+        return NULL;
+
+    g_atomic_int_inc(&cbd->ref_count);
+

i)  If I'm a new contributor, unfamiliar with ofono, and I see this then
I think:  "oh oh, this is a thread-safe library... better be
careful."... and then I get all confused because nothing else is handled
in a thread-safe manner.
ii)  The atomic operations introduce an unnecessary memory barrier.
I _presume_ that the code is written this way because it was inherited
from connman and connman made an early attempt to be thread safe...???
I'm not sure that legacy is worth preserving.
Anyway, enough said on the matter.  The implementation is fine aside
from my reservations.

So let me make you happy:

For all new code the policy is that g_atomic should not be used. Once
we switch to ell for oFono 2.x (or even before that) we should have no
instances of g_atomic in the codebase.

If someone wants to start removing some of these now, be my guest.

Regards,
-Denis