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… :
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… :
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… :
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 <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 21 Jan 2025 13:11:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>