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`. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33083 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I3a639e6896cc3b3fc8e9b2e1a58254710efa0d3f Gerrit-Change-Number: 33083 Gerrit-PatchSet: 11 Gerrit-Owner: arehbein <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: daniel <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Comment-Date: Sun, 18 Jun 2023 09:40:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Gerrit-MessageType: comment
