[L] Change in osmocom-bb[master]: ASCI: Add ASCI notification support to RR layer - gerrit-log

27 Sep 2023

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 &lt;andreas(a)eversberg.eu&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-Attention: laforge &lt;laforge(a)osmocom.org&gt;
Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt;
Gerrit-Comment-Date: Wed, 27 Sep 2023 12:50:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt;
Gerrit-MessageType: comment