Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12227 )
Change subject: ACL: integrate sanitize check into sgsn_acl_* functions ...................................................................... Patch Set 5: > > Patch Set 5: Code-Review-1 > > > > -1 since last version still don't apply my comments regarding > movement of sanitize out of sgsn_acl_* callees. > > You haven't stated why you think it's better (unless I've missed it > somehow). So the answer is still the same (and still in commit > message): having this check in vty makes it hard to unit-test, > having it as a static function next to sgsn_acl_* makes it > unit-testable. > > I don't see any advantage in keeping code which is not directly > related to vty inside sgsn_vty.c. Having sanitize function right > next to where its result is used is way more intuitive and easier > to maintain. Plus the advantage of unit testing. I did, maybe it was not clear enough: "I think sanitizing the imsi should be done by caller of sgsn_acl_* based on where the information come from (from the wire or from known sanitized source)." If you know your data is sane there's no need to re-sanitize it. You should expect the caller of a data struct to provide sane data instead of internally sanitizing it and storing different data from what was provided. It's responsibility of the caller (vty code for instance) to make sure parse of human input is correctly parsed and sanitized. You can do checks inside the data structure if you want (I wouldn't), but I'd avoid changing content of the data being handled in there. -- To view, visit https://gerrit.osmocom.org/12227 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3dff108148683b107e9edac430a0475283580e9 Gerrit-Change-Number: 12227 Gerrit-PatchSet: 5 Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-CC: Stefan Sperling <[email protected]> Gerrit-Comment-Date: Tue, 11 Dec 2018 12:22:14 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No
