Attention is currently required from: arehbein, fixeria, daniel. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33083 ) Change subject: gsm/ipa: Add segmentation callback ...................................................................... Patch Set 8: (5 comments) File src/gsm/ipa.c: https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/a870c855_c921fab6 PS8, Line 728: * -EIO, if the header declares a payload too large */ */ on the next line https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/8df9a684_074de13b PS8, Line 729: int ipa_segmentation_cb(struct msgb *msg) as per struct osmo_io_ops documentation: /*! call-back function to segment the data at message boundaries. * Needs to return the size of the next message. If it returns * -EAGAIN or a value larger than msgb_length() (message is incomplete) * osmo_io will wait for more data to be read. Other negative values * cause the msg to be discarded. */ int (*segmentation_cb)(struct msgb *msg); https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/2253840c_fe1fc679 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 the block). Then, you could already use sizeof(*hh) in line 731, instead of using different things for sizeof() as in line 737 which makes it confusing to read. https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/a30a90cf_89d1a9d8 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. https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/eb741a5b_9095490b 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. 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? -- 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: 8 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: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Attention: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Comment-Date: Thu, 08 Jun 2023 16:13:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment