Attention is currently required from: laforge, pespin, 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 16:
(9 comments)
File src/sccp_scoc.c:
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/cef92837_7aa7730a PS14, Line 1110: xua_opt_data_send_cache(conn, SUA_CO_CORE, xua->hdr.msg_class);
I think I'm missing a point in here. […]
maybe i was misreading this code. It looks to me like below scu_gen_encode_and_send(N_CONNECT, CONFIRM) *sends* a Conn Conf; that made me assume this here is the side that received the Conn Req and is responding with a CC. Then again above case "CC_IND" looks like this *receives* a Conn Conf, didn't see that before.. now i'm confused between the prims, which side is which.
File src/sccp_scoc.c:
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/b76d4a42_2892dd9f PS15, Line 642: /* optional: importance */
The RLC does not have optional importance field.
like i said before, a patch should ideally do one thing. To fix other bits along with it, just put it in a separate commit. it is an act of courtesy to code reviewers; it is harder to read a complex patch when unrelated fixes go with it. it also makes it possible to work with patches as "atoms", though we hardly ever need that part of it.
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/e9d63318_a58c7883 PS15, Line 635: LOGP(DLSCCP, LOGL_ERROR, "replacing unsent %u bytes of optional data cache with %s optional data\n",
AFAIU that's something coming from outside, from a peer, so not really an error of the program itsel […]
I understand, this case is where we want to cache optional data, but there already is other data in the cache. Is this even possible to happen? ... maybe if we send a CR to the peer, but receive no response, and try again with the same CR. Then we already have data in the cache that could not be sent -- so seeing this error means that it is a software bug, did not clean up unsent cached data after a failure??
IMHO a comment and the log message could clarify this, like "ERROR: caching optional data for N-CONNECT, but there already is optional data occupying the cache. Dropping previous data."
ERROR category is good.
File src/sccp_scoc.c:
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/264d5726_895dcaeb PS16, Line 592: nt exp_type could you explain a situation where there might be a mismatch from the expected type? Isn't it always DT1 to be sent at the right time?
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/24694b57_c30425dc PS16, Line 638: msgb_trim(conn->opt_data_cache, 0); (i'd prefer sanitation: msgb_free() here, and always alloc a new one.)
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d76a2fe0_c812277d PS16, Line 667: see Figure C.3 / Q.714 (sheet 2 of 6) */ (osmocom asks to put '*' on every line of comment)
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d0fb57e1_5cc3e68f PS16, Line 673: if (xua_drop_data_check_drop(prim, SCCP_MAX_DATA, "cache overrun")) the optional data is larger than the upper limit of an SCCP DATA section -- this is not related to cache at all, the conn is still open and no cache is ever involved. it is a protocol error caused by the caller. Is it even possible / likely?
Also not an overrun. There is plenty of space in a msgb, there is no previous data in the cache...
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/2949a077_4679ee42 PS16, Line 675: /* There's no need to cache the optional data since the connection is still active at this point */ /* Send the Optional Data in a DT1 ahead of the RLSD, because it is too large to be sent in one message. */
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/c84d1859_9f367b02 PS16, Line 712: ount (again modifying unrelated comments)