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 <msur...@sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanits...@sysmocom.de>
Gerrit-Reviewer: neels <nhofm...@sysmocom.de>
Gerrit-CC: laforge <lafo...@osmocom.org>
Gerrit-Attention: neels <nhofm...@sysmocom.de>
Gerrit-Attention: msuraev <msur...@sysmocom.de>
Gerrit-Comment-Date: Sun, 21 Aug 2022 17:57:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Reply via email to