Attention is currently required from: wbokslag.

laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/33943 )

Change subject: Added first steps towards tetra crypto support ......................................................................

Patch Set 3:

(9 comments)

Patchset:

PS3: I feel a bit sorry for the large number of comments. Don't treat them as strict change requests, more like "these are my thoughts, do with them what you think'. The only ones I think are blockers is the talloc vs. calloc use and the license/copyright header.

File src/crypto/tetra_crypto.h:

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/e26312ab_fed450a8 PS3, Line 47: mcc it's been a long time since I read TETRA specs. However, in GSM there is a difference between MNC 02 and 002, and we made the early mistake to represent them as integers, where we cannot differentiate those two distinct values. We then had to invent work-arounds, etc.

So if TETRA also has similar differences (you tell me!) in terms of number of digits / leading zeroes, it might be best to avoid int.

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/d9329000_417adb5d PS3, Line 54: int is_gssi_range; : int start; : int end; are there situations where those integers really can be negative?

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/46c19233_b00a3a79 PS3, Line 61: mnc is there a reeason to use int for mnc/mcc in 'struct network_info' but uint32_t here?

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/db06bc10_6ecaf02e PS3, Line 71: int num_keys; I guess all of these could be unsigned as they simply count the number of list elements? Using the more constrained unsigned avoids situations where it's unclear what the meaning of a negative number could mean in this context, and it also helps to avoid static analysis tools to "think" about the question of negative values where none are desired/useful.

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/318f3980_df2ccedc PS3, Line 93: const char *tetra_get_key_type_name(int key_type); didn't you define enums above for those types? Wondering why the argument type is int instead of the enum

File src/crypto/tetra_crypto.c:

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/aa304436_7ca02f87 PS3, Line 14: struct tet I'd generally suggest to a void global variables for state that might exist multiple times (think of decrypting multiple different calls/subscribers/... simultaneously).

This is why I'm using a context pointer (like 'struct tetra_mac_state *) that gets passed around to functions. So future software can have multiple of those around, rather than assuming there is only a singleton.

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/989c662a_b8a769cc PS3, Line 82: calloc we generally use talloc in the existing osmo-tetra code base, and it would make sense to not mix different memory allocators. This way it's clear one always uses one type of free() function, and not another.

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/f991f1b1_bf3f500d PS3, Line 96: k->mcc, k->mnc, k->key_type); we have something like OSMO_STRBUF_APPEND() in osmocom/core/utils.h to avoid the manual size-counting + pointer arithmetic in the snprintf calls here.

Not critical, up to you.