1 Apr
2016
1 Apr
'16
11:45 a.m.
Hi 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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)