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:
(2 comments)
Patchset:
PS4:
> I really love this patch! […]
resolving, see separate open logging context discussion.
Patchset:
PS5:
> my -1 point is removing of logging context that was present before this patch.
resolving, see separate open logging context discussion.
--
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:12:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Attention is currently required from: daniel, neels, pespin.
osmith has posted comments on this change by pespin. ( https://gerrit.osmocom.org/c/osmo-mgw/+/39352?usp=email )
Change subject: mgcp_msg: Improve logging checking MGCP line param
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
> this patch might be a misunderstanding from CR on the CRCX parsing patch?
For others reading along, Neels is referring to the discussion here: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/4..6/src/libosmo-mgcp/mgcp_ms…
> for me the important part is the logging context -- layering is only secondary, mentioned to serve adding logging context without layer violations.
I'm trying to follow, but I don't get what you mean:
Old code:
```c
if (endp)
LOGPENDP(endp, DLMGCP, LOGL_NOTICE, "wrong MGCP option format: '%s'\n", line);
else
LOGPTRUNK(trunk, DLMGCP, LOGL_NOTICE, "wrong MGCP option format: '%s'\n", line);
```
New code:
```c
LOGP(DLMGCP, LOGL_NOTICE, "%s: wrong MGCP option format: '%s'\n",
pdata->epname, line);
```
I've looked at what `LOGPENDP` does, and all it does is add the endpoint name:
```
#define LOGPENDP(endp, cat, level, fmt, args...) \
LOGP(cat, level, "endpoint:%s " fmt, \
endp ? endp->name : "none", \
## args)
```
So... it seems the end result is the same? In any case, I don't think it is worth blocking the bigger patch over this, I'll add my +1 there.
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/39352?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: I18fd316a2d830b9418a806e32f27558d980259d6
Gerrit-Change-Number: 39352
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 21 Jan 2025 13:11:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
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>
Attention is currently required from: osmith.
pespin has posted comments on this change by osmith. ( https://gerrit.osmocom.org/c/osmo-ci/+/39389?usp=email )
Change subject: coverity: use proper asn1c branch
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/osmo-ci/+/39389?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-ci
Gerrit-Branch: master
Gerrit-Change-Id: I556b0cc038121b9405556f316d71c0fc75f7d177
Gerrit-Change-Number: 39389
Gerrit-PatchSet: 3
Gerrit-Owner: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 21 Jan 2025 12:59:41 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
osmith has posted comments on this change by osmith. ( https://gerrit.osmocom.org/c/osmo-ci/+/39389?usp=email )
Change subject: coverity: use proper asn1c branch
......................................................................
Set Ready For Review
--
To view, visit https://gerrit.osmocom.org/c/osmo-ci/+/39389?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-ci
Gerrit-Branch: master
Gerrit-Change-Id: I556b0cc038121b9405556f316d71c0fc75f7d177
Gerrit-Change-Number: 39389
Gerrit-PatchSet: 3
Gerrit-Owner: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Comment-Date: Tue, 21 Jan 2025 12:55:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Attention is currently required from: pespin.
Hello Jenkins Builder, daniel, laforge, neels,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-mgw/+/39240?usp=email
to look at the new patch set (#6).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
The change is no longer submittable: Verified is unsatisfied now.
Change subject: mgw: DLCX: Split mgcp header pars parsing into a previous step
......................................................................
mgw: DLCX: Split mgcp header pars parsing into a previous step
Do most of the initial parsing and verification in a prior step, filling
in a "parsed" struct which simplifies logic coming after for different
message types.
Change-Id: I557a3a257ddefedc479a4aff974a74c4e4e2c85d
---
M src/libosmo-mgcp/mgcp_protocol.c
1 file changed, 38 insertions(+), 49 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/40/39240/6
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/39240?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I557a3a257ddefedc479a4aff974a74c4e4e2c85d
Gerrit-Change-Number: 39240
Gerrit-PatchSet: 6
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-Attention: pespin <pespin(a)sysmocom.de>
