Attention is currently required from: fixeria, pespin, daniel.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33083 )
Change subject: gsm/ipa: Add segmentation callback ......................................................................
Patch Set 11:
(8 comments)
File src/gsm/ipa.c:
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/334d140f_25a985ca PS8, Line 728: * -EIO, if the header declares a payload too large */
*/ on the next line
hmm weird, I remember reading somewhere that Osmocom comments do *not* put the comment end marker on the same line. Admittedly, I don't know where I read it, but I'm seeing the same style in lots of other places.
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/f3c4e7d7_562b1daa PS8, Line 729: int ipa_segmentation_cb(struct msgb *msg)
as per struct osmo_io_ops documentation: […]
So what's wrong with the Doxygen style comment? It doesn't contradict the description you quote.
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/1442daa4_d178cad3 PS8, Line 735: const struct ipaccess_head *hh = (const struct ipaccess_head *) msg->data;
You could declare this at the start of the function (we usually declare variables at the start of th […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/ff847965_719776da PS8, Line 737: size_t total_len = payload_len + sizeof(*hh);
"sizeof(*hh) + payload_len;" it's logically easier to understand, as in lefto-to-right order filling […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/e0a2a000_2d5aa618 PS8, Line 738: if (msgb_tailroom(msg) + msgb_length(msg) < total_len) {
iiuc the problem here is that the allocated msgb space is not going to be enough to fit in what IP […]
Done
File src/gsm/ipa.c:
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/e3da8242_e9830cbf PS10, Line 733: osmo_ntohs(hh->len);
Now the problem is that you're accessing the buffer before checking if there is enough data in it.
ah yeah thanks, had that in mind, but forgot it again in between all the other CR comments/changes and rebasing.
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/675c57e2_dea17eb5 PS10, Line 735: msgb_length(msg) + msgb_tailroom(msg);
This is incorrect. […]
I think you got confused here about `msg->data_len`, `msg->len` and `msgb_length()`.
`msg->data_len` includes all the bytes allocated (including tailroom and headroom); `msg->len` includes all the bytes reserved for the message to be stored (i.e. not the tailroom and not the headroom). At least that's what I'm getting from the code.
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/148de202_bc80783f PS10, Line 744: EIO
`ENOMEM` or `ENOSPC` is a better fit here, IMO.
I took a look at `errno -l` and found `ENOBUFS 105 No buffer space available`.
-
arehbein