Attention is currently required from: fixeria, laforge, pespin. jolly 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 4: (10 comments) File src/host/layer23/src/mobile/gsm48_rr.c: https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/2ea223db_328299ed PS3, Line 1331: uint8_t *gcr > `const` Done https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/c1d325d6_c57fd497 PS3, Line 1387: uint8_t *gcr > `const` Done https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/6353e06c_db7eb247 PS3, Line 1399: uint8_t *gcr, uint8_t *ch_desc > both `const` Done https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/7de4a3fd_ab743348 PS3, Line 1440: return -ENOMEM; > You may be leaking the memory allocated by `asci_notif_alloc()` here. […] The allocation is not leaked. There is a timer running for every notification. When it times out, the notification is removed from the list. The list is used to notify the upper layer only once and not for every repeated notification received. https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/771e41fd_de780833 PS3, Line 1461: 5*8 > The comment states 36 bits, but you allocate (5 * 8) bytes? […] Right! https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/8b92d2f5_dddd7c8c PS3, Line 1467: 3*8 > Same here, `OSMO_BYTES_FOR_BITS(24)`, because: […] Done https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/c27d4c86_c0d5ef12 PS3, Line 1473: \n > This is going to break the logstring formatting, better use just one `\n`. Done https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/d7ab4cb8_f8dce05d PS3, Line 1482: if (chd_bv) > `bitvec_free()` is NULL safe, so these checks are redundant. Done https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/cd6e9007_fe204bc9 PS3, Line 1494: msgb_l3len(msg) - sizeof(*sgh); > Is it guaranteed by the caller that `msgb_l3len(msg)` is always `>= > sizeof(*sgh)`? […] Yes it is. The header is checked in the calling function. Here the header is skipped for getting the payload. https://gerrit.osmocom.org/c/osmocom-bb/+/34530/comment/e8d874d0_214649dc PS3, Line 1519: msgb_l3len(msg) - sizeof(*nn); > Same here. There is no check. I will submit another patch that checks the existence of the header for all CCCH messages. This will ensure that at least the NCH header exits in the message. -- 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: 4 Gerrit-Owner: jolly <andr...@eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanits...@sysmocom.de> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Attention: laforge <lafo...@osmocom.org> Gerrit-Attention: fixeria <vyanits...@sysmocom.de> Gerrit-Attention: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Wed, 27 Sep 2023 12:50:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanits...@sysmocom.de> Gerrit-MessageType: comment
