Hi all,
I'm currently working on 2G to 3G TrFO voice negotiation. In practice, that means, to be able to negotiate a set of AMR rates via SDP. And that, in turn, means that we handle fmtp properly in SDP strings.
SDP is not only used in SIP to the other call leg, but also in the MGCP to the media gateway. - So we have a bunch of SDP and fmtp related code in osmo-mgw.git. - Separately, we have an SDP implementation in osmo-msc.git, because the SDP code in osmo-mgw.git was too tightly coupled with other code to be able to reuse it for MNCC->SIP. For fmtp negotiation, I now want similar things in both mgw and msc.
What I want to do now, is move the SDP implementation from osmo-msc.git to osmo-mgw.git/libosmo-mgcp-client and make it public API: a common place that will also enable all other MGCP clients to work with AMR fmtp.
All of this was just the intro =) the question I would like to put out there is about API design: static or dynamic, and opaque or transparent. Am I overthinking this? Or would you like to overthink it with me?
== static or dynamic ==
In osmo-msc.git, the SDP structs so far are fully static and self-contained. The cool thing is that they can be copied around quite easily and there is no need for passing talloc ctx pointers, no need to worry about ownership, memleaks and double frees. But it also means that it uses fixed sizes: - char encoding_name[16], - char fmtp[256], - struct sdp_audio_codec codecs[32] and so on.
Unfortunately, RFC-8866 does not define any limits whatsoever on number of characters or number of codecs in an SDP message. Quoting: "although no length limit is given, it is recommended that they be short" Thanks for nothing! >:(
So for static API design, we need to now pick a good size for every single item in advance.
When fmtp has max 256 bytes and codec lists have max 32 entries, even an empty list will use 32 * 256+ bytes: lots of heap allocations.
And the limits are hard: anyone wanting to correctly express more than 32 different codec variants can simply not use this library.
Dynamic seems a much better fit for RFC-8866. There is practically no limit at all on string length / list length. Also the caller allocates not one byte more memory than is actually needed.
The argument has been going back and forth in my head for weeks. Static makes things so much simpler. But what sizes should we dictate. Ok then dynamic. But these days we have "infinite" memory, the bottleneck is CPU time, so if we can be static, we are gracious on very cheap real estate (memory) while saving a lot of the precious stuff (cycles). Ok then static. But with all those AMR variants that are coming up, I want to at least be able to represent all of them in one list, so I need at least 64 entries. And let's allow long fmtp strings, who knows. Then we'll end up with ~20kb of data that every codec list out there will use up, even if it is empty. Even if it is just a temporary local variable. Even if it is just dragged along for a rare use case and never actually needed. Is it really faster to allocate 100 times more memory statically versus allocating just enough but dynamically?
In the end I decided for dynamic allocation. When I started adding talloc ctx args, I wanted the API to be simple, so every single part of the puzzle is now its own dynamic allocation:
struct osmo_sdp_msg { char *username; // talloc_strdup() char *session_name; // talloc_strdup() [...] struct osmo_sdp_codec_list *codecs; // osmo_sdp_codec_list_alloc() // llist_head // -> osmo_sdp_codec_alloc() // ___ char *encoding_name -> talloc_strdup() // _ char *fmtp -> talloc_strdup() // -next-> osmo_sdp_codec_alloc() // ___ ... // -next-> osmo_sdp_codec_alloc() // ___ ... };
All of this used to be just a single self-contained struct.
You might ask, why is 'codecs', which is just the llist_head, a separate allocation? Because I want the API to trivially figure out the talloc parent context to allocate items from:
osmo_codec_list_add(my_list, my_codec);
and not have to worry about the individual code paths' parent context to allocate new entries from:
osmo_codec_list_add(&my_obj->my_list, my_obj->backpointer.backpointer.ctx, my_codec);
So the only functions with a talloc ctx argument are osmo_codec_alloc(ctx) and osmo_codec_list_alloc(ctx)
This style has a tendency to spread: Now I want to also make osmo-msc's struct codec_filter a dynamic allocation, because the codec_filter_foo() currently have no ctx argument; they still use the static SDP structs. When the codec_filter pointer itself is the neat logical talloc parent of the codec lists it uses, then I don't need to add a separate ctx arg everywhere now.
Am I taking this too far now? Am I overthinking it and either way is fine?
== opaque vs transparent ==
I can either have the full struct definition in the .h file = transparent, or I can hide the struct in the .c file and provide lots of getter and setter functions = opaque.
Both ways have a major advantage over the other:
- opaque means that we can freely extend the API without any ABI breakage, ever.
- transparent means that callers can easily define const arrays of data structures. In my case, I want to keep const lists of const codecs for e.g. test data in unit tests, for listing known codecs in codec_mapping.c, for defining default codec configuration.
transparent allows:
const struct osmo_sdp_codec ran_defaults = { { .encoding_name = "AMR", .rate = 8000, .fmtp = "octet-align=1;mode-set=0,2,4,7", .payload_type = 112, }, { .encoding_name = "AMR", .rate = 8000, .fmtp = "octet-align=1;mode-set=0,2,4", .payload_type = 112, }, { .encoding_name = "AMR", .rate = 8000, .fmtp = "mode-set=0,2,4,7", .payload_type = 112, }, { .encoding_name = "AMR", .rate = 8000, .fmtp = "mode-set=0,2,4", .payload_type = 112, }, { .encoding_name = "GSM-EFR", .rate = 8000, .payload_type = 110, }, { .encoding_name = "GSM", .rate = 8000, .payload_type = 3, }, { .encoding_name = "GSM-HR-08", .rate = 8000, .payload_type = 111, }, }; int i; for (i = 0; i < ARRAY_SIZE(ran_defaults); i++) osmo_sdp_codec_list_add(g_ran_codecs_cfg, &defaults[i])
printf("%s", codec->encoding_name); if (!strcmp(codec->encoding_name, "AMR")) printf(":%s", codec->fmtp);
same in opaque:
osmo_sdp_codec_list_add(g_ran_codecs_cfg, "AMR", 8000, "octet-align=1;mode-set=0,2,4,7", 112); osmo_sdp_codec_list_add(g_ran_codecs_cfg, "AMR", 8000, "octet-align=1;mode-set=0,2,4", 112); osmo_sdp_codec_list_add(g_ran_codecs_cfg, "AMR", 8000, "mode-set=0,2,4,7", 112); osmo_sdp_codec_list_add(g_ran_codecs_cfg, "AMR", 8000, "mode-set=0,2,4", 112); osmo_sdp_codec_list_add(g_ran_codecs_cfg, "GSM-EFR", 8000, NULL, 110); osmo_sdp_codec_list_add(g_ran_codecs_cfg, "GSM", 8000, NULL, 3); osmo_sdp_codec_list_add(g_ran_codecs_cfg, "GSM-HR-08", 8000, NULL, 111);
printf("%s", osmo_sdp_codec_get_encoding_name(codec)); if (!strcmp(osmo_sdp_codec_get_encoding_name(codec), "AMR")) printf(":%s", osmo_sdp_codec_get_ftmp(codec));
The opaque looks alright in comparison, but - being able to enlist a struct osmo_sdp_codec in a const array is powerful, allowing the paradigm to replace code complexity by good data structures. So used in codec_mapping.c in osmo-msc. - requiring getters and setters for each end every member creates a long list of API functions: 'get' and 'set' for each orthogonal member; we might be tempted to "dupe" many llist_*() functions for osmo_sdp_codec_list. - having to call functions for each end every member makes the code a lot more noisy. - it forces all allocation decisions down onto the caller = only using the API author's favorite dynamic allocation, hello memleaks etc.
IMHO, opaque APIs are often lots of code with very low density of actually important stuff.
/* This function gets foo. */ struct foo *get_foo(x) { return x->foo; }
This is so plump and boring!!
Currently I am following the transparent way, and putting 'bool v2;' extension flags for a distant future in the structs' ends.
But there has been a discussion here that the truly good APIs are opaque. talloc, libSDL, libsndfile come to mind.
== both ==
Finally, these two aspects are interdependent: An opaque API *has* to be dynamic.
So far, I am moving from the transparent+static implementation to transparent+dynamic.
Should I also go the "next" step to opaque+dynamic? I don't really want to...
I've had these considerations many times in my life, and there never seems to be the one best way. Thanks for your thoughts!
If you need more code reference for what I am talking about:
old static: https://cgit.osmocom.org/osmo-msc/tree/include/osmocom/msc/sdp_msg.h?id=1b1a... https://cgit.osmocom.org/osmo-msc/tree/src/libmsc/sdp_msg.c?id=1b1a39bea1c52...
new dynamic+transparent: https://cgit.osmocom.org/osmo-mgw/tree/include/osmocom/sdp?h=neels/sdp&i... https://cgit.osmocom.org/osmo-mgw/tree/src/libosmo-sdp?h=neels/sdp&id=24...
~N
Hi Neels,
not sure what kind of answer you expect here, probably just some sort of poll since most of the "rant" was already provided :)
As usual, I'd go for opaque. Since opaque means the implementation can be changed, then it doesn't matter much whether you want to use dynamic or static. It can be a mix of both, use whatever fits more in each place. But I'd definitely make the lib allocate the structs and have them as opaque pointers.
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.
Regards, Pau
Yeah I am having the landscape of choices mapped out, and I'd like to get a feel for the general opinion to fine tune my heading. "At what degree of which aspects do you guys see the optimum?"
On Mon, Feb 26, 2024 at 10:57:16AM +0100, Pau Espin Pedrol wrote:
As usual, I'd go for opaque. Since opaque means the implementation can be changed, then it doesn't matter much whether you want to use dynamic or static. It can be a mix of both, use whatever fits more in each place.
An opaque API has to be dynamic. There is always a struct foo * that was returned by a dynamic foo_alloc().
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
That means I want to allocate an llist_head instance dynamically. Going this way makes a lot more, small dynamic allocations.
(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.
At the moment I want to go for many small dynamic allocations. Fine?
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/code... 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.
(In my current status, osmo_sdp_codec is transparent. If the opinions are strong I'll add effort to make it opaque)
Another idea would be to invent a separate SDP struct in osmo-msc, but that also needs more API to convert one to the other... I don't want that.
Thanks!! ~N
Hi,
On 2/26/24 19:36, Neels Hofmeyr wrote:
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.
Yes.
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?
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.
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/code... 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
-
Neels Hofmeyr -
Pau Espin Pedrol