Attention is currently required from: daniel, laforge, neels, pespin. osmith 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 7: Code-Review+1 (3 comments) File src/libosmo-mgcp/mgcp_msg.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/956f1254_9fe9a0ad?usp=email : PS4, Line 168: } > (is this function 1:1 unchanged? if the function had not moved, it would be > trivially visible from t […] in patchset 7, the endpoint name was added again to the log message https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/39a6b407_d5ef5c55?usp=email : PS4, Line 210: goto mgcp_header_done; > non-functional cosmetic issue resolving (let's get this big, almost 1 month patch out of the queue!) https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/e620e535_a57af8c8?usp=email : PS4, Line 257: LOGP(DLMGCP, LOGL_NOTICE, "wrong MGCP option format: '%s'\n", line); I've looked at https://gerrit.osmocom.org/c/osmo-mgw/+/39352, see my comment there. > 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. In the patch here, we now have one log message without the endp name (and there was another one in in parse_x_osmo_ign(), which has been fixed in patch version 7). This log message gets removed, and replaced with one that has the endp name in follow-up patch https://gerrit.osmocom.org/c/osmo-mgw/+/39352. There is a separate open discussion about less logging context in osmux_init(), but Pau said he'll address that in follow-up patches too. In my opinion it is not worth blocking this almost 1 month old patch for this. So it looks like we can resolve at least the discussion here. -- 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: 7 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-Reviewer: osmith <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Comment-Date: Tue, 21 Jan 2025 13:11:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: pespin <[email protected]>
