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) -- 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: 16 Gerrit-Owner: msuraev <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Attention: msuraev <[email protected]> Gerrit-Comment-Date: Thu, 01 Sep 2022 11:27:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: msuraev <[email protected]> Gerrit-MessageType: comment
