Attention is currently required from: daniel, laforge, pespin. neels has posted comments on this change by pespin. ( https://gerrit.osmocom.org/c/osmo-mgw/+/39224?usp=email ) Change subject: mgw: CRCX: Split mgcp header pars parsing into a previous step ...................................................................... Patch Set 5: Code-Review-1 (11 comments) Patchset: PS5: my -1 point is removing of logging context that was present before this patch. File include/osmocom/mgcp/mgcp_protocol.h: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/ea1670fe_385aa8c5?usp… : PS4, Line 14: bool have_sdp; a response of "i don't care" for your own naming choices usually goes with accepting CR suggestions...? https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/924dde05_026f7e64?usp… : PS4, Line 27: .x_osmo_ign = 0, ack, grep is a good point https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/5cdd2768_71ccbf6d?usp… : PS4, Line 34: char *save; hmm, but you are taking local variables and putting them in a struct, so this is a good time to choose names? i agree that avoiding renames makes the patch diff easier to read. Would be great to have useful names in the end. File src/libosmo-mgcp/mgcp_msg.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/b5717a7b_03529081?usp… : PS4, Line 173: int mgcp_parse_hdr_pars(struct mgcp_parse_data *pdata) ack https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/2814809a_32f62a4d?usp… : PS4, Line 180: e( ack, i will not bother https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/8d2efa47_7d42e29e?usp… : PS4, Line 208: case '\0': ack https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/871f1297_92134113?usp… : PS4, Line 212: LOGP(DLMGCP, LOGL_NOTICE, "CRCX: unhandled option: '%c'/%d\n", *line, *line); ack https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/813d32fa_2d5fe38d?usp… : PS4, Line 257: LOGP(DLMGCP, LOGL_NOTICE, "wrong MGCP option format: '%s'\n", line); What i would like to see is that the parsing code returns error messages to the caller. Then the caller should log the error message with the relevant context added. What i don't want to see is that the parsing code does a lot of context-less logging of errors, and the user needs to guess from adjacent logging where the error originated from. (The linked patch also does context-less logging) I will veto removing logging context that was present before this patch. File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/6b03f072_23137015?usp… : PS4, Line 710: if (osmux_init(trunk) < 0) { I agree that the endp is only marginally related and could be omitted from logging context. But then the LOGPTRUNK() below should be done within osmux_init(). thanks! https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/8ac1ecd5_f1ae0d62?usp… : PS4, Line 846: case -523: (you don't agree?) -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/39224?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I3ee5158c254213203830fe9c38de11c15b4b19c1 Gerrit-Change-Number: 39224 Gerrit-PatchSet: 5 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Comment-Date: Thu, 16 Jan 2025 12:20:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels &lt;nhofmeyr(a)sysmocom.de&gt; Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;