[S] Change in libosmocore[master]: gsm/ipa: Add segmentation callback - gerrit-log

18 Jun 2023

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 &lt;arehbein(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-CC: daniel &lt;dwillmann(a)sysmocom.de&gt;
Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Attention: daniel &lt;dwillmann(a)sysmocom.de&gt;
Gerrit-Comment-Date: Sun, 18 Jun 2023 09:40:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;
Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-MessageType: comment