Attention is currently required from: fixeria, msuraev. neels 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 14: Code-Review-1 (11 comments) Patchset: PS14: (marked less important comments as resolved, right from the start) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d07fbf8a_7ab12466 PS6, Line 583: /* Cache the optional data (if necessary) and return indication whether cache was used */

It would create unnecessary inconvenience for the caller. […] you are by definition fiddling with the data when dropping the optional data.

ah ok i see, you pass a bool into the encoding function instead of modifying the source message. (I think it's a good idea to modify the message to be sent, plus some other design choices that could be more clear, but i'm bike-shedding) https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/e756994a_deca92c8 PS6, Line 1003: /* N. B: we've ignored CREF sending errors as there's no recovery option anyway */

Why unrelated? It's certainly worth it to explain why in the above case we use cache while in here w […] (would be nicer to have a separate patch for adding missing comments)

File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/a531d541_42daaa19 PS8, Line 659: break

Yes? drop the "break;"

File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/3c0f008d_16bdbde9 PS14, Line 594: sua should this be m3ua? @laforge? https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/a1bf4766_f1391f10 PS14, Line 653: static bool xua_opt_data_length_lim(struct sccp_connection *conn, const struct osmo_scu_prim *prim, int msg_type) (this function is decribed as checking a length and returning a bool, but there is an actual data transmisison triggered in here. It would be good to have a clear name and API comment.) https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/6d7de0c4_b74d1033 PS14, Line 675: There's no need to cache the optional data since the connection is still active at this point (is this comment accurate? i don't understand it... so the situation here is A B ------> DT1 ------> RLSD right? how about "Directly send Optional Data first, if it does not fit in the RLSD msg" ) https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/605c6ef9_1a395e16 PS14, Line 713: /* optional: importance */ (would be nicer to have a separate patch for adding missing comments) https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/4b0eab21_2197bd12 PS14, Line 734: /* optional: importance */ (would be nicer to have a separate patch for adding missing comments) https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/efb08c53_8a3dfae9 PS14, Line 789: /* optional: importance */ (would be nicer to have a separate patch for adding missing comments) https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/0b91622a_b0ab061a PS14, Line 1110: xua_opt_data_send_cache(conn, SUA_CO_CORE, xua->hdr.msg_class); I think this is mixed up somehow. This is about too much data in the Connection Request. It should be: A B ------> Conn Req (cache Optional Data) <------ Conn Conf ------> DT1 with cached data This code here looks like A B ------> Conn Req <------ DT1 with cached data (there is no cached data??) <------ Conn Conf -- 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: 14 Gerrit-Owner: msuraev <msuraev@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy@sysmocom.de> Gerrit-Attention: msuraev <msuraev@sysmocom.de> Gerrit-Comment-Date: Wed, 24 Aug 2022 23:59:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> Comment-In-Reply-To: msuraev <msuraev@sysmocom.de> Gerrit-MessageType: comment