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 -- 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: 6 Gerrit-Owner: msuraev <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Comment-Date: Sun, 21 Aug 2022 12:57:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Gerrit-MessageType: comment
