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=1b1… https://cgit.osmocom.org/osmo-msc/tree/src/libmsc/sdp_msg.c?id=1b1a39bea1c5… new dynamic+transparent: https://cgit.osmocom.org/osmo-mgw/tree/include/osmocom/sdp?h=neels/sdp&… https://cgit.osmocom.org/osmo-mgw/tree/src/libosmo-sdp?h=neels/sdp&id=2… ~N -- - Neels Hofmeyr &lt;nhofmeyr(a)sysmocom.de&gt; http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschäftsführer / Managing Directors: Harald Welte