Attention is currently required from: laforge, pespin, fixeria, msuraev.
9 comments:
File src/sccp_scoc.c:
Patch Set #14, 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:
Patch Set #15, 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.
Patch Set #15, 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:
Patch Set #16, 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?
Patch Set #16, Line 638: msgb_trim(conn->opt_data_cache, 0);
(i'd prefer sanitation: msgb_free() here, and always alloc a new one.)
Patch Set #16, Line 667: see Figure C.3 / Q.714 (sheet 2 of 6) */
(osmocom asks to put '*' on every line of comment)
Patch Set #16, 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...
Patch Set #16, 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. */
(again modifying unrelated comments)
To view, visit change 29084. To unsubscribe, or for help writing mail filters, visit settings.