Attention is currently required from: wbokslag.
laforge has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-tetra/+/34001 )
Change subject: Added stub for decryption and full keystream application functions
......................................................................
Patch Set 2:
(2 comments)
File src/crypto/tetra_crypto.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/34001/comment/87f12f9f_714dba42
PS2, Line 207: case KSG_TEA1:
: tea1_stub(iv, eck, num_bytes, ks_bytes);
: break;
: case KSG_TEA2:
: tea2_stub(iv, eck, num_bytes, ks_bytes);
: break;
: case KSG_TEA3:
: tea3_stub(iv, eck, num_bytes, ks_bytes);
: break;
I think it's a bit weird to add the stub functions that don't do anything in a
commit here. This leads to situations that if somebody calls the function now, it returns
success (true) despite not having generated the keystream. I think it would be wise to
not merge the stub functions at all, and have generate_keystream run into the
default-clause below which gives a proper error message and return code instead of silent
failure.
https://gerrit.osmocom.org/c/osmo-tetra/+/34001/comment/cea1cccd_ff18f74c
PS2, Line 305: /
we typically prefer /* */ comments. Feels especially inconsistenc since this very same
patch is adding multiple comments in different styles.
--
To view, visit
https://gerrit.osmocom.org/c/osmo-tetra/+/34001
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-tetra
Gerrit-Branch: master
Gerrit-Change-Id: I4e6147f206ad6046f32e08015ec9721b64382ca1
Gerrit-Change-Number: 34001
Gerrit-PatchSet: 2
Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Comment-Date: Sat, 29 Jul 2023 08:37:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment