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