<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 5: Code-Review-1<br>> -1 since last version still don't apply my comments regarding<br>movement of sanitize out of sgsn_acl_* callees.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">You haven't stated why you think it's better (unless I've missed it<br>somehow). So the answer is still the same (and still in commit<br>message): having this check in vty makes it hard to unit-test,<br>having it as a static function next to sgsn_acl_* makes it<br>unit-testable.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't see any advantage in keeping code which is not directly<br>related to vty inside sgsn_vty.c. Having sanitize function right<br>next to where its result is used is way more intuitive and easier<br>to maintain. Plus the advantage of unit testing.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">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)."</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p><a href="https://gerrit.osmocom.org/12227">View Change</a></p><ul style="list-style: none; padding: 0;"></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12227">change 12227</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/12227"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-sgsn </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ic3dff108148683b107e9edac430a0475283580e9 </div>
<div style="display:none"> Gerrit-Change-Number: 12227 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 11 Dec 2018 12:22:14 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>
<div style="display:none"> Gerrit-HasLabels: No </div>