Attention is currently required from: neels. msuraev has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 )
Change subject: SIGTRAN: cache Optional Data for SCCP CR and CC ......................................................................
Patch Set 6:
(14 comments)
This change is ready for review.
File src/sccp_scoc.c:
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/52a2d23a_0f5b8ac5 PS6, Line 675: ional: import
unrelated change?
I think it's related: we arrange comments to match the sequence of message parts.
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/fe3442cf_63806b97 PS6, Line 104: uint8_t opt_data_len;
please store a msgb with the data to be cached. […]
I don't think it's worth it bothering with dynamic allocation for overhead of 2.5Mb per 10k connections but ok.
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/a60f79d3_14b207b2 PS6, Line 105: int opt_data_cached_from;
opt_data_cached_msgtype ... […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/9bfc7ac9_6bb0e3c7 PS6, Line 106:
if it still needs multiple elements, i would prefer […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d3877f3b_fb6084e3 PS6, Line 583: /* Cache the optional data (if necessary) and return indication whether cache was used */
"return true if the data was cached" […]
It would create unnecessary inconvenience for the caller. Also, I think it's a bad idea to modify message to-be-sent - I'd rather keep current const to corresponding parameter and avoid unnecessary fiddling with the data.
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/f6eddae7_f0388241 PS6, Line 595: void xua_opt_data_clear_cache(struct sccp_connection *conn)
this patch is only editing sccp_scoc. […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/481c4328_582cf55a PS6, Line 619: /* Check optional Data size limit, cache if necessary, return indication whether original data should be sent */
"return true if..." […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/c92d621b_739e4b4d PS6, Line 633: return !xua_opt_data_cached(conn, prim, msg_type); : break;
drop these two lines, the cases are identical
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d4bcd1c8_50920ef4 PS6, Line 639: if (msgb_l2len(prim->oph.msg) > SCCP_MAX_OPTIONAL_DATA) {
would prefer if only one function implements the actual length check (code dup)
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/7b193378_296fc679 PS6, Line 641: "dropping optional data with length %u exceeding max of %u according to ITU-T Rec. Q.713 §4.4\n",
dropping? so the data never gets sent? could make sense for a Connection-Refused, but explaining tha […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/56c48d64_b5232bb1 PS6, Line 646: case SUA_CO_RELRE: /* §4.5 Released (RLSD) */
i think the idea here is to first send a DT1 with the data that is too large and then sending the "e […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/73df0ac0_d51100d2 PS6, Line 986: struct xua_common_hdr hdr = XUA_HDR(SUA_MSGC_CO, SUA_CO_COAK);
only hdr. […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/7df461c1_54e6f805 PS6, Line 1003: /* N. B: we've ignored CREF sending errors as there's no recovery option anyway */
"COREF" ? […]
Why unrelated? It's certainly worth it to explain why in the above case we use cache while in here we don't.
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/802dad94_deceb681 PS6, Line 1027: xua_opt_data_clear_cache(conn);
seems that this cleanup should be done in the fsm's cleanup() cb and in the IDLE stat's onenter() fu […]
Done
-
msuraev