This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Harald Welte laforge at gnumonks.orgHi Neels, On Fri, Apr 01, 2016 at 12:49:13AM +0200, Neels Hofmeyr wrote: > First off, we allocate 1024 bytes of msgb. Into it goes an osmo_scu_prim > struct (here 'prim'). We set some parameters in prim, fair enough. But > now what, prim->oph, the prim header, has a msg pointer, which points back > at the msgb that contains the prim. really?? The msgb containing a struct > pointing back at the same msgb has my head spinning. I actually think it's a pretty straight-forward solution when thinking of of the general problem and the constraints: * we already had 'msgb' handling everywhere in our message processing code and were using it quite happily to pass messages around. * at inter-layer boundaries in the ISO/OSI model, you have primitives that consist of the primitive itself, as well as the payload data. The primitive consists of some operation like N-DATA, plus the associatted request/indication/confirmation/response part. A primitive occors on a given SAP. This was put into struct osmo_prim_hdr * In order to not have the primitive separate and out-of-band, we store it inside the msgb itself. In early versions of some code, we had the osmo_prim_hdr allocated separately and passed both it and the msgb to every function. That was a bit cumbersome and error-prone. * To avoid all the primitive-handling code having to derive the pointer to osmo_prim_hdr as the first line of code, we pass around a pointer to the osmo_prim_hdr itself, rather than a pointer to msgb. This also makes the code more explicit in that you can clearly see that it doesn't expect a generic msgb, but that it deals with a primitive as argument. * As the osmo_prim_hdr is not stored at a fixed offset inside the msgb, we cannot use tricks like 'offset_of()' to get back from the osmo_prim_hdr to the msgb. Thus we store the pointer to the msgb header inside the osmo_prim_hdr. The knowledge of the msgb is needed particularly in case the primitive needs to be free()d. > Finally we add the data, l2h pointing at the start of it. I'm confused ... > how large is the msgb supposed to be? Is it len + sizeof(*prim)? yes, at the very minimum it has to be that size. If you need to determine that frequently, we could add an inline function or macro to the code. > Secondly, let's look at: > > int osmo_sccp_tx_unitdata_msg(struct osmo_sua_link *link, > const struct osmo_sccp_addr *calling_addr, > const struct osmo_sccp_addr *called_addr, > struct msgb *msg) > { > int rc; > > rc = osmo_sccp_tx_unitdata(link, calling_addr, called_addr, > msg->data, msgb_length(msg)); > msgb_free(msg); > > return rc; > } > So, something out there has a msgb containing data. For example, from > ranap_new_msg_paging_cmd(). That msgb has probably been allocated > specifically to compose a message to go over the sccp link. > > We go on, though, to allocate *another* msgb (size 1024, remember), and > memcpy the first msgb into the second msgb, behind the osmo_scu_prim > struct. > > Are there good reasons for this? This function was just a quick wrapper around the already-existing osmo_sccp_tx_unitdata() function, avoiding a copy+paste of that funtion's body. > I would have envisioned something like: tell ranap_new_msg_paging_cmd() to > allocate sizeof(*prim) of headroom; or first create a msgb of fixed > known-max size, and add first the prim header and then compose the data. the problem particularly when interfacing with asn1c generated code is that you end up doing so many copies and dynamic allocations all the time, that it probably hardly matters anymore. It's one hell of inefficiency, at least for from where we come from. > And, given the union prim->u has "unknown" size, why not point at the data > from prim->oph directly, instead of pointing back at the msgb? Or have > that union first? That would be easier to understand. Now you are mixing up the SCCP-User pimitives (osmo_scu_prim) with the generic underlying osmo_prim_hdr layer. osmo_prim_hdr is used in other contexts, including osmo-tetra, for which I first developed it, AFAIR. It is also used in libosmogb, osmocom-bb. It should IMHO be used much more generically at inter-layer boundaries, but a lot of the exiting code pre-dates osmo_prim_hdr. From my point of view, every layer boundary should be using the osmo_prim_hdr construct and associated service access points. To get back to your question: * SCCP Use SAP: prim->u doesn't have unknown size, but has the size of the largest member in the union. You should always allocate memory for sizeof(prim). * prim->oph (osmo_prim_hdr) pointing to the message data is way too restrictive. How would you go back to 'msg' in order to free it? How would yo get to all the other meta-data of a msg that it carries around, like msgb->dst, the header pointers to variuos layers, the msgb->cb information, ..? > If some code needs to know the msgb that contains the prim->oph, I find it > weird to pass it the prim->oph with a backpointer, instead of just passing > the msgb right away. I find that much more convenient instead of having an extra msgb argument to each and every function dealing with osmo_prim_hdr. The goal should be to make it easy for the programmer to write code that is readable, rather than passing around a second pointer that points virtually to the same chunk of data. It also poses the danger of a caller passing a pointer to an oph not inside the msgb (or vice versa), with desastrous results. The only case where you as a user of the primitives really have to worry about it is when you msgb_copy() the msgb holding a primitive. But then, if your get passed an osmo_prim_hdr as argument, you should use a (to be written) dedicated osmo_prim_copy() function, which understands the peculiarities of osmo_prim and performs a msgb_copy() with oph->msg pointer adjustment after the copy. calling msgb_copy() on a oph->msg is just not allowed. But I don't think we have ever needed that so far, that's why the osmo_prim_copy() doesn't exist yet. Regards, Harald -- - Harald Welte <laforge at gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)