Hi list,
osmo_prim_init() and msgb use firstly confuses me, and secondly is
apparently "inefficient" in sccp_helpers.c.
First, let's look at osmo_sccp_tx_unitdata() (libosmo-sccp branch
sysmocom/iu):
int osmo_sccp_tx_unitdata(struct osmo_sua_link *link,
const struct osmo_sccp_addr *calling_addr,
const struct osmo_sccp_addr *called_addr,
uint8_t *data, unsigned int len)
{
struct msgb *msg = msgb_alloc(1024, "sccp_tx_unitdata");
struct osmo_scu_prim *prim;
struct osmo_scu_unitdata_param *param;
prim = (struct osmo_scu_prim *) msgb_put(msg, sizeof(*prim));
param = &prim->u.unitdata;
memcpy(¶m->calling_addr, calling_addr, sizeof(*calling_addr));
memcpy(¶m->called_addr, called_addr, sizeof(*called_addr));
osmo_prim_init(&prim->oph, SCCP_SAP_USER, OSMO_SCU_PRIM_N_UNITDATA, PRIM_OP_REQUEST, msg);
msg->l2h = msgb_put(msg, len);
memcpy(msg->l2h, data, len);
return osmo_sua_user_link_down(link, &prim->oph);
}
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.
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)?
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.
I'll first use this as-is, talking about premature optimization. But I'd
prefer to have neither the wicked cunning nor the blunt dumbness in there;
the combination strikes me as jolly mad ;)
Are there good reasons for this?
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.
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.
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.
~Neels
