Attention is currently required from: fixeria, msuraev.
Patch set 14:Code-Review -1
11 comments:
Patchset:
(marked less important comments as resolved, right from the start)
File src/sccp_scoc.c:
Patch Set #6, 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)
Patch Set #6, 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:
Patch Set #8, Line 659: break
Yes?
drop the "break;"
File src/sccp_scoc.c:
should this be m3ua? @laforge?
Patch Set #14, 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.)
Patch Set #14, 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" )
Patch Set #14, Line 713: /* optional: importance */
(would be nicer to have a separate patch for adding missing comments)
Patch Set #14, Line 734: /* optional: importance */
(would be nicer to have a separate patch for adding missing comments)
Patch Set #14, Line 789: /* optional: importance */
(would be nicer to have a separate patch for adding missing comments)
Patch Set #14, 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 change 29084. To unsubscribe, or for help writing mail filters, visit settings.