26 Feb
2024
26 Feb
'24
8:42 p.m.
Hi,
On 2/26/24 19:36, Neels Hofmeyr wrote:
Yes.
I see no problem with returning an llist_head (when it makes sense to do
so).
I'd say it really depends on the very specific use case.
An opaque API has to be dynamic.
There is always a struct foo * that was returned by a dynamic foo_alloc().
Yeah, sure, I meant the structs below those, the ones which are/may be
allocated internally.
But I'd definitely make the lib allocate the
structs and have them as opaque
pointers.
Ok, poll taken: the benefit of easily modifying the API later weighs more than
having to write dozens of foo_get_x() and foo_set_x() and calling functions all
the time instead of using plain pointer arithmetic. Regarding "benefits of iterating over
llist", you can simply have an API
return the llist pointer and have some API to get the object from the llist
per-item pointer which is internally implemented by means of container_of()
or a cast or similar.
Yes. But is it really opaque to return an llist_head? Should the API maybe hide
the particular list implementation and provide functions for walking the list?
What I'd also like your opinion on is, how far would you take the dynamic-ness?
Is it fine to have many detailed allocation "stages", for the benefit of logic
and opacity? Example:
I like it when the object itself is the talloc context for its members:
struct codec_list *my_list = codec_list_alloc(ctx);
codec_list_add(my_list, item); // <-- use my_list as talloc parent for the new
entry
Again I'd say this kind of decision is really specific to each use case,
depending on the relations between objects, ownserships, etc.
I'd say in general it's fine as long as parent/child relations between
them are clear, so that the child makes no sense out of the parent.
That means I want to allocate an llist_head instance dynamically. Going this
way makes a lot more, small dynamic allocations.
I think you actually want to say is (or I want to see it this way): "I
want to allocate an object which is a container of other items".
That object being implemented as a llist (having a field llist_head) is
an implementation detail. The fact that you add an API to return an
pointer to an llist_head which allows iterating over the items contained
in the parent object is *not* an implementation detail, it's simply an
API of the object.
(benefit: the logical structure of the data shows very nicely in the talloc
reports!)
An alternative would be: use an llist_head that the caller simply has as static
member of its state:
struct llist_head my_list;
codec_list_add(&my_list, ctx, item); // <-- always provide the talloc ctx
separately
It has less separate dynamic allocations, but there are more ways to shoot
oneself in the foot, and more ctx arguments all over the API.
Also it's not really opaque anymore.
It really depends on the usercase. If you really only want an llist
which is not to be managed from the library, then you could go this way.
But then freeing everything becomes more cumbersome.
In general I'd gor for the other example you shared first.
At the moment I want to go for many small dynamic allocations.
Fine?
You can always add an object pool if small dynamic allocations become a
problem (malloc locks, fragmentation, etc.).
The good part is that this is all implementation details encapsulated in
the allocation functions.
Ah and also, when the osmo_sdp_codec{} is opaque, how would we set up the data
structures in codec_mapping.c? I guess with a function that procedurally builds
an allocated list? Currently it's an (IMHO nice) const array of pure data:
https://gerrit.osmocom.org/plugins/gitiles/osmo-msc/+/master/src/libmsc/cod…
If struct osmo_sdp_codec is transparent, it can stay this way. If I make it
opaque, every part here that says .sdp = {...} would have to be dynamically
created via API instead.
I see not much of a problem with it. That's created once at startup anyway?
If you need to refer to it from different places, it can be global and
just take the pointer to one of them.
(In my current status, osmo_sdp_codec is transparent. If the opinions are
strong I'll add effort to make it opaque)
My opinion/experience is in general go for opaque, specially for complex
objects/structures which can be updated in the future.
And from the start, add a _internal.h header and add there all function
declarations there until you see they are really needed somewhere
outside of the library.
Regards,
Pau
--
- Pau Espin Pedrol <pespin(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte