Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
msuraev has submitted this change. (
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 )
Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
..
SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
The length limit of optional Data parameter is 130 bytes according to ITU-T Rec
Q.713 §4.2..§4.5. If we receive CR, CC or
RLSD message with bigger data - cache it if necessary and send via separate DT1
message after connection becomes active.
Fixes: OS#5579
Change-Id: I0033faf9da393418930252233ce74d62cd1cef8a
---
M src/sccp_scoc.c
1 file changed, 130 insertions(+), 13 deletions(-)
Approvals:
Jenkins Builder: Verified
pespin: Looks good to me, but someone else must approve
laforge: Looks good to me, but someone else must approve
osmith: Looks good to me, but someone else must approve
msuraev: Looks good to me, approved
diff --git a/src/sccp_scoc.c b/src/sccp_scoc.c
index 2ed72ee..cff6e80 100644
--- a/src/sccp_scoc.c
+++ b/src/sccp_scoc.c
@@ -49,12 +49,13 @@
#include
#include
+#include
#include
#include
#include
#include
#include
-
+#include
#include
#include
#include
@@ -99,6 +100,8 @@
uint32_t sccp_class;
uint32_t release_cause; /* WAIT_CONN_CONF */
+ struct msgb *opt_data_cache;
+
/* incoming (true) or outgoing (false) */
bool incoming;
@@ -514,10 +517,20 @@
return conn_create_id(user, conn_id);
}
+static void conn_opt_data_clear_cache(struct sccp_connection *conn)
+{
+ if (conn->opt_data_cache) {
+ msgb_free(conn->opt_data_cache);
+ conn->opt_data_cache = NULL;
+ }
+}
+
/* destroy a SCCP connection state, releasing all timers, terminating
* FSM and releasing associated memory */
static void conn_destroy(struct sccp_connection *conn)
{
+ conn_opt_data_clear_cache(conn);
+
conn_stop_connect_timer(conn);
conn_stop_inact_timers(conn);
conn_stop_release_timers(conn);
@@ -575,11 +588,112 @@
return 0;
}
+/* Send cached optional data (if any) from expected message type and clear
cache */
+static void xua_opt_data_send_cache(struct sccp_connection *conn, int
exp_type, uint8_t msg_class)
+{
+ const struct xua_dialect *dialect = &xua_dialect_sua;
+ const struct xua_msg_class *xmc = dialect->class[msg_class];
+
+ if (!conn->opt_data_cache)
+ return;
+
+ if (conn->opt_data_cache->cb[0] != exp_type) {
+ /* Caller (from the FSM) knows what was the source of Optional
Data we're sending.
+* Compare this information with source of Optional Data
recorded while caching
+* to make sure we're on the same page.
+*/
+ LOGP(DLSCCP, LOGL_ERROR, "unexpected message type %s != cache
source %s\n",
+xua_class_msg_name(xmc, exp_type),
xua_class_msg_name(xmc, conn->opt_data_cache->cb[0]));
+ } else {
+ osmo_sccp_tx_data(conn->user, conn->conn_id,
msgb_data(conn->opt_data_cache), msgb_length(conn->opt_data_cache));
+ }
+
+ conn_opt_data_clear_cache(conn);
+}
+
+/* Check if optional data should be dropped, log given error message if so */
+static bool xua_drop_data_check_drop(const struct osmo_scu_prim *prim,
unsigned lim, const char *message)
+{
+ if (msgb_l2len(prim->oph.msg) > lim) {
+ LOGP(DLSCCP, LOGL_ERROR,
+"%s: dropping optional data with length %u > %u -
%s\n",
+osmo_scu_prim_name(&prim->oph),
msgb_l2len(prim->oph.msg), lim, message);
+ return true;
+ }
+ return false;
+}
+
+/* Cache the optional data (if necessary)
+ * returns true if Optional Data should be kept while encoding the message */
+static bool xua_opt_data_cache_keep(struct sccp_connection *conn, const struct
osmo_scu_prim *prim, int msg_type)
+{
+ uint8_t *buf;
+
+ if (xua_drop_data_check_drop(prim, SCCP_MAX_DATA, "cache overrun"))
+ return false;
+
+ if (msgb_l2len(prim->oph.msg) > SCCP_MAX_OPTIONAL_DATA) {
+ if (conn->opt_data_cache) {
+ /* Caching optional data, but there already is optional
data occupying the cache: */
+ LOGP(DLSCCP, LOGL_ERROR, "replacing unsent %u bytes of
optional data cache with %s optional data\n",
+msgb_length(conn->opt_data_cache),
osmo_scu_prim_name(&prim->oph));
+ msgb_trim(conn->opt_data_cache, 0);
+ } else {
+ conn->opt_data_cache = msgb_alloc_c(conn,
SCCP_MAX_DATA, "SCCP optional data cache for CR/CC/RLSD");
+ }
+
+ buf = msgb_put(conn->opt_data_cache, msgb_l2len(prim->oph.msg));
+ memcpy(buf, msgb_l2(prim->oph.msg), msgb_l2len(prim->oph.msg));
+
+ conn->opt_data_cache->cb[0] = msg_t
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, fixeria. msuraev 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 17: Code-Review+2 -- 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: 17 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: msuraev Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Attention: neels Gerrit-Attention: fixeria Gerrit-Comment-Date: Fri, 09 Sep 2022 09:41:02 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, fixeria, msuraev. osmith 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 17: Code-Review+1 -- 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: 17 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Attention: neels Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Thu, 08 Sep 2022 12:49:55 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, fixeria, msuraev. laforge 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 17: Code-Review+1 -- 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: 17 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Attention: neels Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Thu, 08 Sep 2022 12:46:03 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, fixeria, msuraev. pespin 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 17: Code-Review+1 -- 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: 17 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Wed, 07 Sep 2022 11:35:04 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, pespin, fixeria. Hello Jenkins Builder, neels, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 to look at the new patch set (#17). Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD .. SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD The length limit of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or RLSD message with bigger data - cache it if necessary and send via separate DT1 message after connection becomes active. Fixes: OS#5579 Change-Id: I0033faf9da393418930252233ce74d62cd1cef8a --- M src/sccp_scoc.c 1 file changed, 130 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/84/29084/17 -- 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: 17 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-MessageType: newpatchset
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, pespin, fixeria. msuraev 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: (14 comments) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/e310bcbb_d100f350 PS14, Line 1110:xua_opt_data_send_cache(conn, SUA_CO_CORE, xua->hdr.msg_class); > maybe i was misreading this code. […] Ack File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/fe2b440c_1c650313 PS15, Line 642: /* optional: importance */ > like i said before, a patch should ideally do one thing. […] Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/260bad71_8dd729c7 PS15, Line 600: if ( > Simply drop it then. Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/050a586c_5d9ec7ac PS15, Line 606: } else > not critical, but IMHO there should be curly braces. […] Ack https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/73da6543_4b9fb0b5 PS15, Line 606: } else > not critical, but IMHO there should be curly braces. […] Ack https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/3a90071f_211ad50f PS15, Line 635: LOGP(DLSCCP, LOGL_ERROR, "replacing unsent %u bytes of optional data cache with %s optional data\n", > I understand, this case is where we want to cache optional data, but there > already is other data in […] Ack https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/87ac74a3_b8be3def PS15, Line 635: LOGP(DLSCCP, LOGL_ERROR, "replacing unsent %u bytes of optional data cache with %s optional data\n", > I understand, this case is where we want to cache optional data, but there > already is other data in […] Ack https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/7138c7ce_a9fe6137 PS15, Line 784: xua_msg_add_sccp_addr(xua, SUA_IEI_DEST_ADDR, &conn->calling_addr); > Then write a new comit "comments are placed to match the fields order in the > spec. […] Done File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/b8ad7bb9_9260bd9a PS16, Line 592: nt exp_type > could you explain a situation where there might be a mismatch from the > expected type? Isn't it alway […] Both expected types refer to the message which was source of the optional data. This check is simply an additional safeguard to ensure we hadn't screwed up while adjusting FSM. The caching happens in one place, sending in another but we always know what kind of Optional Data we're about to send (i. e. from which message it was cached). So here we compare the type of the message as recorded while caching with the type of the message FSM thinks it's sending. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/441d202f_57a781cb PS16, Line 638: msgb_trim(conn->opt_data_cache, 0); > (i'd prefer sanitation: msgb_free() here, and always alloc a new one. […] I don't: if msgb_trim() is implemented properly there should be no difference in our case (fixed buffer size) besides wasted CPU cycle on unnecessary function calls. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d2098370_4a00e0bd PS16, Line 667:see Figure C.3 / Q.714 (sheet 2 of 6) */ > (osmocom asks to put '*' on every line of comment) Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/994a5e32_94ddf4ec 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 c […] Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/5099038f_2e5fe8c1 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 mess […] Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/b2b8d5d3_9fe7a406 PS16, Line 712: ount > (again modifying unrelated comments) 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: 16 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-Comment-Date: Wed, 07 Sep 2022 10:37:23 + Gerrit-HasComments: Yes G
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: pespin, fixeria, msuraev. laforge 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: (1 comment) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/69fe6950_b4dba128 PS15, Line 606: } else > "Do not unnecessarily use braces where a single statement will do. […] not critical, but IMHO there should be curly braces. The linux kernel coding style should state that whenever a single statement is broken into several lines, the block uses braces. It should also state that if either of the two 'else' clauses use braces, both get them. Just quoting from memory here. -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Mon, 05 Sep 2022 13:02:18 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: laforge Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Thu, 01 Sep 2022 11:27:45 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels Comment-In-Reply-To: pespin Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, fixeria, Max. pespin 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: (4 comments) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/a81464a3_aa110981 PS15, Line 600: if ( > We shouldn't. […] Simply drop it then. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d28a52f7_7f4401a2 PS15, Line 606: } else > Actually it's better to add them to else according to https://www.kernel. […] "Do not unnecessarily use braces where a single statement will do." So why do you say it's better according to it? https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/cd5f094b_282695b0 PS15, Line 635: LOGP(DLSCCP, LOGL_ERROR, "replacing unsent %u bytes of optional data cache with %s optional data\n", > I think it's error - this situation should not arise normally. AFAIU that's something coming from outside, from a peer, so not really an error of the program itself. Not important though. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/68333f0c_d0c3678f PS15, Line 784: xua_msg_add_sccp_addr(xua, SUA_IEI_DEST_ADDR, &conn->calling_addr); > Similar to the above: the comments are placed to match the fields order in > the spec. Then write a new comit "comments are placed to match the fields order in the spec." -- 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: Max Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Attention: Max Gerrit-Comment-Date: Tue, 30 Aug 2022 16:14:53 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge Comment-In-Reply-To: pespin Comment-In-Reply-To: Max Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, pespin, fixeria. Hello Jenkins Builder, neels, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 to look at the new patch set (#16). Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD .. SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD The length limit of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or RLSD message with bigger data - cache it if necessary and send via separate DT1 message after connection becomes active. While at it, clarify the order of comments in the encoding routine to match the spec. Fixes: OS#5579 Change-Id: I0033faf9da393418930252233ce74d62cd1cef8a --- M src/sccp_scoc.c 1 file changed, 133 insertions(+), 16 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/84/29084/16 -- 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: Max Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-MessageType: newpatchset
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, pespin, fixeria. Max 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 15: (8 comments) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/4363d4d6_ffda9ece PS15, Line 642: /* optional: importance */ > why is this line removed here? this is non related. The RLC does not have optional importance field. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/b870683f_95186f62 PS15, Line 600: if ( > Ack We shouldn't. Shall I replace it with assert or just drop? https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/6fa8a826_fd35be4a PS15, Line 606: } else > remove curly braces in the if. Actually it's better to add them to else according to https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/51617eeb_e7923d15 PS15, Line 630: if (xua_drop_data_check(prim, SCCP_MAX_DATA, "cache overrun")) > ... […] Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/f4acfafc_5ad77dad PS15, Line 635: LOGP(DLSCCP, LOGL_ERROR, "replacing unsent %u bytes of optional data cache with %s optional data\n", > NOTICE I think it's error - this situation should not arise normally. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/da5d2afe_a69997e6 PS15, Line 639: msgb_alloc(SCCP_MAX_DATA, "SCCP optional data cache for CR/CC/RLSD"); > Ack Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/5d2bd69d_a2630137 PS15, Line 639: msgb_alloc(SCCP_MAX_DATA, "SCCP optional data cache for CR/CC/RLSD"); > Ack Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/699f9d67_3b1a53c1 PS15, Line 784: xua_msg_add_sccp_addr(xua, SUA_IEI_DEST_ADDR, &conn->calling_addr); > why is this line removed here? this is non related. Similar to the above: the comments are placed to match the fields order in the spec. -- 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: 15 Gerrit-Owner: Max Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: pespin Gerrit-Attention: fixeria Gerrit-Comment-Date: Tue, 30 Aug 2022 16:07:09 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, fixeria, msuraev. pespin 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 15: (7 comments) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/99187a07_486bf644 PS15, Line 642: /* optional: importance */ why is this line removed here? this is non related. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/22584621_ed05b35e PS15, Line 600: if ( > in whihc situation would we have a zero-length opt_data_cache? Ack https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/95f59ccb_f6e0401e PS15, Line 606: } else remove curly braces in the if. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/76465829_27cd148a PS15, Line 630: if (xua_drop_data_check(prim, SCCP_MAX_DATA, "cache overrun")) ...check_drop(), otherwise it's not clear what you are checking and it's confusing since you actually check for drop and return the opposite to keep it. Actually, since you call this function _keep(), it would make sense to reverse the loginc on the xua_drop_data_check() function... I would even drop the function completely tbh. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/1e4a2119_e4a3eef7 PS15, Line 635: LOGP(DLSCCP, LOGL_ERROR, "replacing unsent %u bytes of optional data cache with %s optional data\n", NOTICE https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/fba00b9f_9b6214fd PS15, Line 639: msgb_alloc(SCCP_MAX_DATA, "SCCP optional data cache for CR/CC/RLSD"); > If we ever should run into memory leaks, it would be better to use > msgb_alloc_c(conn) to attach this […] Ack https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/d53cddbe_6bbb5c85 PS15, Line 784: xua_msg_add_sccp_addr(xua, SUA_IEI_DEST_ADDR, &conn->calling_addr); why is this line removed here? this is non related. -- 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: 15 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-CC: pespin Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Tue, 30 Aug 2022 11:49:41 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, fixeria, msuraev. laforge 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 15: (2 comments) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/aebf41e5_35d43381 PS15, Line 600: if ( in whihc situation would we have a zero-length opt_data_cache? https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/59cd70d2_3640867b PS15, Line 639: msgb_alloc(SCCP_MAX_DATA, "SCCP optional data cache for CR/CC/RLSD"); If we ever should run into memory leaks, it would be better to use msgb_alloc_c(conn) to attach this msgb to the 'conn'. -- 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: 15 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Mon, 29 Aug 2022 15:03:27 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, fixeria. msuraev 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: (1 comment) Patchset: PS14: > I've added comment to the line which raised questions - hopefully this would > clarify things. 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: 14 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Comment-Date: Mon, 29 Aug 2022 10:26:12 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, fixeria. Hello Jenkins Builder, neels, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 to look at the new patch set (#15). Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD .. SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD The length limit of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or RLSD message with bigger data - cache it if necessary and send via separate DT1 message after connection becomes active. While at it, clarify the order of comments in the encoding routine to match the spec. Fixes: OS#5579 Change-Id: I0033faf9da393418930252233ce74d62cd1cef8a --- M src/sccp_scoc.c 1 file changed, 132 insertions(+), 16 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/84/29084/15 -- 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: 15 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-MessageType: newpatchset
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge, fixeria. msuraev 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: (4 comments) This change is ready for review. Patchset: PS14: I've added comment to the line which raised questions - hopefully this would clarify things. File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/6ce89fd4_8d05fbff PS8, Line 659: break > drop the "break;" Done File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/48dbaa89_d31e82c7 PS14, Line 603: if (conn->opt_data_cache->cb[0] != exp_type) > cosmetic: we use curly braces around multi-line blocks, even if it's only a > single statement within […] Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/a86dafd8_0ce12659 PS14, Line 1110:xua_opt_data_send_cache(conn, SUA_CO_CORE, xua->hdr.msg_class); > or maybe this? […] I think I'm missing a point in here. Could you elaborate - what exactly you think should be changed in here and why? That's both the intention behind this code and how the result looks like in wireshark: A B --> Conn Req (cache Optional Data) <-- Conn Conf --> DT1 with cached data When the lib receieve CR with way too much data (but not big enough to not fit into DT1) we cache it, send CR without optional data and send DT1 after receiving CC. To make sure we do not interfere with regular user traffic the DT1 (with outstanding optional data from CR) is sent here from conn_pend_out state before we notify lib user that CC was received. You can reproduce this yourself by following readme for examples/ code which is already merged to master. -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: laforge Gerrit-Attention: fixeria Gerrit-Comment-Date: Mon, 29 Aug 2022 10:25:23 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels Comment-In-Reply-To: laforge Comment-In-Reply-To: fixeria Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: fixeria, msuraev. laforge has removed a vote from this change. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 ) Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD .. Removed Code-Review+1 by laforge -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-MessageType: deleteVote
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
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: (1 comment) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/92a02cfd_2bea3b83 PS14, Line 1110:xua_opt_data_send_cache(conn, SUA_CO_CORE, xua->hdr.msg_class); > I think this is mixed up somehow. […] or maybe this? -> Conn Req <- Conn Conf (defer Optional Data) <- DT1 with Optional data then the order would be wrong? -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Thu, 25 Aug 2022 14:40:24 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
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: (1 comment) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/b12c07ee_68eb55ef PS14, Line 594: sua > should this be m3ua? @laforge? there was clarification here: https://gerrit.osmocom.org/c/libosmo-sccp/+/29158 -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Thu, 25 Aug 2022 14:32:43 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: fixeria Gerrit-Attention: msuraev Gerrit-Comment-Date: Wed, 24 Aug 2022 23:59:31 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels Comment-In-Reply-To: fixeria Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, msuraev. laforge 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 (2 comments) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/02ce8f35_533239e2 PS6, Line 675: ional: import > I think it's related: we arrange comments to match the sequence of message > parts. Ack File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/f50a942c_6262958e PS14, Line 603: if (conn->opt_data_cache->cb[0] != exp_type) cosmetic: we use curly braces around multi-line blocks, even if it's only a single statement within the block. that should be linux kernel coding style, too. -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Attention: neels Gerrit-Attention: msuraev Gerrit-Comment-Date: Tue, 23 Aug 2022 17:30:40 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, laforge.
msuraev 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:
(3 comments)
File src/sccp_scoc.c:
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/56fc9585_4b6c502b
PS4, Line 580: if (prim && msgb_l2(prim->oph.msg) &&
msgb_l2len(prim->oph.msg)) {
> exit early […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/74875340_b63c288a
PS4, Line 585: xua_msg_free(xua);
> I agree, it feels rather odd if a function supposedly adding a bit to
> caller-provided 'xua' suddenly […]
Done
https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/8fa01b13_7a79393a
PS4, Line 615: if (!xua_add_data(prim, xua, "4.2"))
> this looks like a mem leak of above xua_msg_alloc(). […]
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: 14
Gerrit-Owner: msuraev
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: neels
Gerrit-CC: laforge
Gerrit-Attention: neels
Gerrit-Attention: laforge
Gerrit-Comment-Date: Tue, 23 Aug 2022 10:15:10 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels
Comment-In-Reply-To: laforge
Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, msuraev. fixeria 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: (1 comment) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/c654ca9a_1f862a77 PS8, Line 603: xua_class_msg_name > I don't think it's worth bothering actually: this log is unlikely to be > triggered in a first place, […] Ack, not a merge blocker. -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-Attention: msuraev Gerrit-Comment-Date: Tue, 23 Aug 2022 10:10:59 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, fixeria. msuraev 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: (1 comment) This change is ready for review. File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/fd72159f_34145962 PS8, Line 603: xua_class_msg_name > Nope, this does now solve the problem. […] I don't think it's worth bothering actually: this log is unlikely to be triggered in a first place, and for static name collision to happen both expected AND current msg_type have to be unknown - which makes it even more unlikely. -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-Attention: fixeria Gerrit-Comment-Date: Tue, 23 Aug 2022 10:09:45 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, msuraev. fixeria 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 13: (1 comment) File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/75dc6945_f9d89ff6 PS8, Line 603: xua_class_msg_name > Done Nope, this does now solve the problem. In your new patchset 'exp_name' still points to the internal static buffer of xua_class_msg_name() and calling xua_class_msg_name() again may overwrite it. A proper solution would be adding xua_class_msg_name_buf(), which would accept the buffer as an argument. You can find some examples by grepping _buf and _c. As a quick solution, you can simply log hex numbers without using xua_class_msg_name(). -- 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: 13 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-Attention: msuraev Gerrit-Comment-Date: Mon, 22 Aug 2022 13:49:28 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: msuraev Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, fixeria. Hello Jenkins Builder, neels, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 to look at the new patch set (#12). Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD .. SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD The length limit of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or RLSD message with bigger data - cache it if necessary and send via separate DT1 message after connection becomes active. While at it, clarify the order of comments in the encoding routine to match the spec. Fixes: OS#5579 Change-Id: I0033faf9da393418930252233ce74d62cd1cef8a --- M src/sccp_scoc.c 1 file changed, 131 insertions(+), 16 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/84/29084/12 -- 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: 12 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-Attention: fixeria Gerrit-MessageType: newpatchset
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, fixeria. msuraev 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 11: (8 comments) Commit Message: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/a3b110f7_3d86edbc PS8, Line 9: Limit length of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or > This reads a bit weird. […] Done File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/f0100bc3_6bdeaec6 PS8, Line 50: #include > cosmetic: move it below to other imports from 'osmocom/core'. Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/ecc3dca8_51692095 PS8, Line 603: xua_class_msg_name > xua_class_msg_name() operates on a static buffer, so calling it more than > once in a logging statemen […] Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/76e546b0_6f13dea0 PS8, Line 613: lim > IIUC, this value is always SCCP_MAX_DATA. […] No, it's not. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/36748543_09562d0c PS8, Line 623: xua_opt_data_cache_check > When I see function names ending with _check, I expect them to do nothing > else but check something a […] Renamed and updated docs for clarity. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/7c59d29f_d77102b1 PS8, Line 650: static bool xua_opt_data_length_check(struct sccp_connection *conn, const struct osmo_scu_prim *prim, int msg_type) > Again, this naming is really confusing. […] Done https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/7a9c0031_0ad8e9b7 PS8, Line 659: break > unreachable Yes? https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/08795265_cff33e10 PS8, Line 1006: xua_opt_data_clear_cache > IMO, this should be done in conn_destroy(). 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: 11 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-Attention: fixeria Gerrit-Comment-Date: Mon, 22 Aug 2022 13:28:23 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels, msuraev. fixeria 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 8: Code-Review-1 (8 comments) Commit Message: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/b4a12965_631453b6 PS8, Line 9: Limit length of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or This reads a bit weird. I suggest to reword this sentence a bit: "The length limit of optional Data parameter is 130 bytes according to ...". File src/sccp_scoc.c: https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/71eac16a_a492bac8 PS8, Line 50: #include cosmetic: move it below to other imports from 'osmocom/core'. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/e492c454_2d79602b PS8, Line 603: xua_class_msg_name xua_class_msg_name() operates on a static buffer, so calling it more than once in a logging statement may result in printing the same value twice. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/6a245579_bbce0965 PS8, Line 613: lim IIUC, this value is always SCCP_MAX_DATA. Do we really need to accept it as an argument? https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/dcfca9b6_4ef859a8 PS8, Line 623: xua_opt_data_cache_check When I see function names ending with _check, I expect them to do nothing else but check something and return some outcome. But here _check actually means populate the cache if necessary and return true if caching is not needed. This is confusing. I suggest to rename it to xua_opt_data_cache_populate() or so, and return true if the cache was populated. https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/106453f9_f4b1f5a4 PS8, Line 650: static bool xua_opt_data_length_check(struct sccp_connection *conn, const struct osmo_scu_prim *prim, int msg_type) Again, this naming is really confusing. _opt_data_length_check implies that this function does check length of optional data and return an outcome. In reality this function not only does the length checks, but may also cache data or even Tx it by calling osmo_sccp_tx_data(). https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/ee25b20a_253a13c4 PS8, Line 659: break unreachable https://gerrit.osmocom.org/c/libosmo-sccp/+/29084/comment/937dacf4_df42e77a PS8, Line 1006: xua_opt_data_clear_cache IMO, this should be done in conn_destroy(). -- 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: 8 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-Attention: msuraev Gerrit-Comment-Date: Sun, 21 Aug 2022 17:57:08 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
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/CC/RLSD .. Set Ready For Review -- 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: 8 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-Comment-Date: Sun, 21 Aug 2022 15:08:38 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in libosmo-sccp[master]: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD
Attention is currently required from: neels. Hello Jenkins Builder, neels, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-sccp/+/29084 to look at the new patch set (#7). Change subject: SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD .. SIGTRAN: cache Optional Data for SCCP CR/CC/RLSD Limit length of optional Data parameter is 130 bytes according to ITU-T Rec Q.713 §4.2..§4.5. If we receive CR, CC or RLSD message with bigger data - cache it if necessary and send via separate DT1 message after connection becomes active. While at it, clarify the order of comments in the encoding routine to match the spec. Fixes: OS#5579 Change-Id: I0033faf9da393418930252233ce74d62cd1cef8a --- M src/sccp_scoc.c 1 file changed, 131 insertions(+), 17 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/84/29084/7 -- 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: 7 Gerrit-Owner: msuraev Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels Gerrit-CC: laforge Gerrit-Attention: neels Gerrit-MessageType: newpatchset
