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=email : PS4, Line 14: bool have_sdp; > I don't care tbh, feel free to submit a patch renaming when you do further > work on related code. 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=email : PS4, Line 27: .x_osmo_ign = 0, > I still prefer having them here explicitly since it provides a quick look > (also through grep) on the […] ack, grep is a good point https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/5cdd2768_71ccbf6d?usp=email : PS4, Line 34: char *save; > I'm simply keeping the previously existing var name. […] 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=email : PS4, Line 173: int mgcp_parse_hdr_pars(struct mgcp_parse_data *pdata) > That's a internal static library of osmo-mgw, so no need for that. ack https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/2814809a_32f62a4d?usp=email : PS4, Line 180: e( > There was a discussion about this recently iirc. Currently we have: […] ack, i will not bother https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/8d2efa47_7d42e29e?usp=email : PS4, Line 208: case '\0': > No idea, simply moving old code here. ack https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/871f1297_92134113?usp=email : PS4, Line 212: LOGP(DLMGCP, LOGL_NOTICE, "CRCX: unhandled option: '%c'/%d\n", *line, *line); > It is further updated in follow-up patch, I tried to do it in steps. ack https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/813d32fa_2d5fe38d?usp=email : PS4, Line 257: LOGP(DLMGCP, LOGL_NOTICE, "wrong MGCP option format: '%s'\n", line); > I submitted https://gerrit.osmocom. […] 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=email : PS4, Line 710: if (osmux_init(trunk) < 0) { > Comment removed by: pespin; Reason: wrong thread 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=email : PS4, Line 846: case -523: > (i humbly favor passing the MGCP result codes as positive numbers, more like > a result code enum, and […] (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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Comment-Date: Thu, 16 Jan 2025 12:20:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: pespin <[email protected]>
