Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13259 )
Change subject: add gsm0808_create_handover_request_ack2 to add AoIP RTP addr ...................................................................... Patch Set 6: This quirk explained in the commit log is a minor detail but it bugs me. If we merge this once, we will always keep this sockaddr_storage as pointer instead of as member chiseled in stone. The long term perspective could have been to also provide these structs like struct gsm0808_handover_request_ack as the output of decoding in libosmocore, instead of just as input to encoding. So far the situation is that libosmocore can only *encode* BSSAP, but *decoding* is done separately in each program. i.e., osmo-bsc implements TLV parsing of some BSSAP messages, while osmo-msc implements TLV parsing for others. Now, if at some point I would add a gsm0808_decode_handover_request_ack() function, I would like to provide struct gsm0808_handover_request_ack as out-argument, and I would also like to then have the sockaddr_storage as direct member. Otherwise the caller would need to first populate with a pointer before calling. All possible of course, but rather quirky IMO. This just because the use of sockaddr_storage isn't properly separated from implementations that get compiled on embedded and can't use it; embedded builds include headers naming struct sockaddr_storage even though the platform doesn't support it, and even function implementations using sockaddr_storage get compiled there but can't ever be used because they have unresolved references to functions handling sockaddr_storage ... meh If we reshuffled the files to separate sockaddr_storage from embedded properly, we could include netinet/in.h directly in gsm0808.h and just use struct sockaddr_* as members. A similar quirk shows up in that osmo_ip_port patch, I have to carefully navigate around what includes what. In the regression tests, it is even weirder. There is an "if ENABLE_MSGFILE" in testsuite.at, but an 'if' like that doesn't seem to have any effect at all. So we can't actually exclude individual tests on embedded. How does that work, we just don't run any tests there apparently? What do you guys think, should I try to separate sockaddr_storage from embedded builds before merging (or releasing) this, or is this a can of worms and incompatibilities that will suck me into realms where I don't want to go? -- To view, visit https://gerrit.osmocom.org/13259 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia71542ea37d4fd2c9fb9b40357db7aeb111ec576 Gerrit-Change-Number: 13259 Gerrit-PatchSet: 6 Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Comment-Date: Fri, 15 Mar 2019 05:05:23 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No
