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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <[email protected]> Gerrit-Attention: wbokslag <[email protected]> Gerrit-Comment-Date: Sat, 29 Jul 2023 08:37:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
