Attention is currently required from: jolly. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/34530?usp=email ) Change subject: ASCI: Add ASCI notification support to RR layer ...................................................................... Patch Set 3: Code-Review-1 (10 comments) File src/host/layer23/src/mobile/gsm48_rr.c: https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/7ad01536_86ad894e PS3, Line 1331: uint8_t *gcr `const` https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/56f2f0ee_e66ae20c PS3, Line 1387: uint8_t *gcr `const` https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/7348a4ae_c9178028 PS3, Line 1399: uint8_t *gcr, uint8_t *ch_desc both `const` https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/1bf90897_7b7b4b41 PS3, Line 1440: return -ENOMEM; You may be leaking the memory allocated by `asci_notif_alloc()` here. Maybe just use `OSMO_ASSERT(nmsg != NULL)`? NULL is unlikely anyway... https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/fc1eebf2_4e6f1fe2 PS3, Line 1461: 5*8 The comment states 36 bits, but you allocate (5 * 8) bytes? I guess you need 5 bytes here, or just `OSMO_BYTES_FOR_BITS(36)`. https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/37c943f4_10f54735 PS3, Line 1467: 3*8 Same here, `OSMO_BYTES_FOR_BITS(24)`, because: ``` \param[in] size Number of bytes in the vector ``` https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/f2581c45_059f561f PS3, Line 1473: \n This is going to break the logstring formatting, better use just one `\n`. https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/87f6829b_24176fad PS3, Line 1482: if (chd_bv) `bitvec_free()` is NULL safe, so these checks are redundant. https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/596ddc34_3dea3de9 PS3, Line 1494: msgb_l3len(msg) - sizeof(*sgh); Is it guaranteed by the caller that `msgb_l3len(msg)` is always `>= sizeof(*sgh)`? If not, we may end up with a negative value here and thus huge bitvec length. AFAICS, it's not (see my comments to 34529). https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/5e22edaa_c1637cdf PS3, Line 1519: msgb_l3len(msg) - sizeof(*nn); Same here. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/34530?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I49df02cb4d99d9aab1ea3ca13beb2ea00ae4c9f4 Gerrit-Change-Number: 34530 Gerrit-PatchSet: 3 Gerrit-Owner: jolly <andreas@eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Reviewer: pespin <pespin@sysmocom.de> Gerrit-Attention: jolly <andreas@eversberg.eu> Gerrit-Comment-Date: Tue, 26 Sep 2023 15:18:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment