Hi Jonas,
On Thu, Oct 25, 2018 at 8:55 AM Jonas Bonn <jonas(a)norrbonn.se> wrote:
Hi Giacinto,
On 25/10/2018 07:03, Giacinto Cifelli wrote:
> +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.
overall I agree with you, but I have no feelings for the code.
If not needed otherwise, I adapt to the various projects I contribute,
in style and preferences.
Here was decided by Marcel that this is the only way, and I adhere to this.
But, since I am investing quite some time on ofono, and it is not a
company internal project,
I plan to write a book on it, where I could also mention that it is
not thread-safe, despite the occasional presence of atomic operations.
/Jonas
Giacinto