Attention is currently required from: neels, msuraev. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 ) Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD ...................................................................... Patch Set 8: Code-Review-1 (8 comments) Commit Message: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/b4a12965_631453b6 PS8, Line 9: Limit length of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or This reads a bit weird. I suggest to reword this sentence a bit: "The length limit of optional Data parameter is 130 bytes according to ...". File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/71eac16a_a492bac8 PS8, Line 50: #include <osmocom/core/msgb.h> cosmetic: move it below to other imports from 'osmocom/core'. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/e492c454_2d79602b PS8, Line 603: xua_class_msg_name xua_class_msg_name() operates on a static buffer, so calling it more than once in a logging statement may result in printing the same value twice. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/6a245579_bbce0965 PS8, Line 613: lim IIUC, this value is always SCCP_MAX_DATA. Do we really need to accept it as an argument? https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/dcfca9b6_4ef859a8 PS8, Line 623: xua_opt_data_cache_check When I see function names ending with _check, I expect them to do nothing else but check something and return some outcome. But here _check actually means populate the cache if necessary and return true if caching is not needed. This is confusing. I suggest to rename it to xua_opt_data_cache_populate() or so, and return true if the cache was populated. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/106453f9_f4b1f5a4 PS8, Line 650: static bool xua_opt_data_length_check(struct sccp_connection *conn, const struct osmo_scu_prim *prim, int msg_type) Again, this naming is really confusing. _opt_data_length_check implies that this function does check length of optional data and return an outcome. In reality this function not only does the length checks, but may also cache data or even Tx it by calling osmo_sccp_tx_data(). https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/ee25b20a_253a13c4 PS8, Line 659: break unreachable https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/937dacf4_df42e77a PS8, Line 1006: xua_opt_data_clear_cache IMO, this should be done in conn_destroy(). -- To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-sccp Gerrit-Branch: master Gerrit-Change-Id: I0033faf9da393418930252233ce74d62cd1cef8a Gerrit-Change-Number: 29084 Gerrit-PatchSet: 8 Gerrit-Owner: msuraev &lt;msuraev(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Attention: msuraev &lt;msuraev(a)sysmocom.de&gt; Gerrit-Comment-Date: Sun, 21 Aug 2022 17:57:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment