Attention is currently required from: fixeria. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/27509 )

Change subject: struct gsm_encr: store alg_id in human-readable format ......................................................................

Patch Set 2: Code-Review-1

(6 comments)

Commit Message:

https://gerrit.osmocom.org/c/osmo-bsc/+/27509/comment/bf470c69_00486e0f PS2, Line 30: ciph_mod_set = (lchan->encr.alg_id << 1) | 1; well, in Cipher Mode Setting, it has to be (a5_n - 1) << 1

File include/osmocom/bsc/gsm_data.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/27509/comment/cea727ab_cb86df91 PS2, Line 613: uint8_t alg_id; alg_id is named after the Algorithm Identifier IE. I'd like to rename this member to 'a5_n' or 'alg_a5_n', because

- we use alg_id in various places, e.g. osmo-msc uses it as N+1, and it also exists in libosmocore

- to me 'a5_n' most clearly indicates which representation it is storing.

- with a rename the patch shows every place in the code that uses this and we can easily verify that all of them are correct.

File src/osmo-bsc/abis_rsl.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/27509/comment/5e15b741_e994f735 PS2, Line 163: switch (*out++) { nitpick (ok, it's correct, but IMHO the code becomes far less head scratchy when we don't ++ inline in conditionals. i'd have put a local variable there / maybe do the switch on the human readable a5_n)

https://gerrit.osmocom.org/c/osmo-bsc/+/27509/comment/542605d6_0f78c2f8 PS2, Line 663: if (lchan->encr.alg_id > 0) { (here is a place where i have to think twice, I thought it is incorrect at first; if the name were a5_n, it would have helped me here)

File src/osmo-bsc/gsm_04_08_rr.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/27509/comment/0617ceb0_a94f7a3d PS2, Line 596: _id "- 1" is missing now! 44.018 10.5.2.9

File src/osmo-bsc/gsm_08_08.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/27509/comment/43598214_90ded639 PS2, Line 88: chosen_encr also rename this to chosen_a5_n?