Attention is currently required from: wbokslag.
9 comments:
Patchset:
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:
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.
int is_gssi_range;
int start;
int end;
are there situations where those integers really can be negative?
is there a reeason to use int for mnc/mcc in 'struct network_info' but uint32_t here?
Patch Set #3, 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.
Patch Set #3, 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:
Patch Set #3, 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.
Patch Set #3, 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.
Patch Set #3, 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.
To view, visit change 33943. To unsubscribe, or for help writing mail filters, visit settings.