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

17 Jun 2023

Attention is currently required from: arehbein, pespin, daniel.

fixeria has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/33083 )

Change subject: gsm/ipa: Add segmentation callback
......................................................................

Patch Set 9:

(3 comments)

File src/gsm/ipa.c:

https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/6e1c17af_700eabb2 
PS2, Line 730: msg->data_len < total_len
...
  Seems better to me, as that would prevent more error
cases Ah, I see. I forgot that `msg->data_len` is the maximum length the buffer
can hold, while `msg->len` is how much is used out of `msg->data_len`. Fine then.

File src/gsm/ipa.c:

https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/dcc21e55_f1eb6463 
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 […] Agreeing with Pau here. There
is no strict requirement to declare variables at the top of the respective block, but in
this specific case we can improve code readability a bit by doing `msgb_length(msg) <
sizeof(*hh)` above.

https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/11bc0665_6da6a3c8 
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 IPA message expects. 
ack.

...
  Talking about that in the log message would be a lot
more usefu for the user? since that probably means the dev has to increase the msgb size
when allocating it? 
Indeed, the logging message should be more specific. Something like `msgb (len=%zu) is not
large enough to fit received IPA message (len=%zu)` maybe?

Also, this is not expected to happen too often, so `OSMO_UNLIKELY`?

-- 
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: 9
Gerrit-Owner: arehbein &lt;arehbein(a)sysmocom.de&gt;
Gerrit-Reviewer: Jenkins Builder
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-CC: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Attention: arehbein &lt;arehbein(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: Sat, 17 Jun 2023 09:48:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein &lt;arehbein(a)sysmocom.de&gt;
Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;
Comment-In-Reply-To: daniel &lt;dwillmann(a)sysmocom.de&gt;
Gerrit-MessageType: comment