15 Jan
2025
15 Jan
'25
5:52 p.m.
Attention is currently required from: 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 4:
(23 comments)
Patchset:
PS4:
I really love this patch!
it feels like seeing your bicycle cleaned and repaired, and the weird noises are gone.
(...except for dropping the logging context part, that feels a bit like removing the
headlight =)
File include/osmocom/mgcp/mgcp_protocol.h:
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/08e29747_e07c1c09?usp… :
PS4, Line 7: #define MGCP_PARSE_HDR_PARS_OSMUX_CID_WILDCARD (-1)
(IMHO enums are favorable to #defines)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/8ad663e1_faf98080?usp… :
PS4, Line 14: bool have_sdp;
(i'd have called it 'sdp_present' -- 'have' sounds to me like
requesting to generate something, not like indicating a parsing result. i see the name
comes from a local var. AFAICT foo_present is the usual naming convention in osmocom... or
might be just wishful thinking.)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/c0b99a6b_f6438f90?usp… :
PS4, Line 27: .x_osmo_ign = 0,
(the " = 0" values are also there implicitly, so it could be just the two .mode
and .remote_osmux_cid lines)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/7fa32c0e_7e294259?usp… :
PS4, Line 34: char *save;
(naming: every struct member saves values -- so save what? maybe mgcp_msg_str?)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/7f900303_e051ed9c?usp… :
PS4, Line 38: /* MGCP Body: */
naming: the "body" in MGCP is the SDP message string starting with a blank line,
so "MGCP Body: mgcp_header_pars" is a logical conflict
File src/libosmo-mgcp/mgcp_msg.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/31ab0de5_26e5bb52?usp… :
PS4, Line 168: }
(is this function 1:1 unchanged? if the function had not moved, it would be trivially
visible from the diff; forward declarations help, or a separate patch for moving first)
EDIT: it is not 1:1 unchanged.
it drops logging context; commented on that in deail further below.
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/3b355116_43719194?usp… :
PS4, Line 170: ha
haders gonna hade
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/4498f117_736d11f7?usp… :
PS4, Line 170: ea
messeage
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/7496fb67_ffd305c0?usp… :
PS4, Line 173: int mgcp_parse_hdr_pars(struct mgcp_parse_data *pdata)
this is not public API, right? so i don't need to ask of you an opaque allocator
function or future-proof structs, right. i'm asking because it's in libosmo-mgcp
-- but that isn't installed, correct?
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/88769a89_6bb45204?usp… :
PS4, Line 179: /* parse CallID C: and LocalParameters L: */
(i guess this comment can go, no useful information conveyed and not even accurate.)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/f0da502d_e3822f1a?usp… :
PS4, Line 180: e(
(i think the linter now usually asks for a space when it is a for-loop style macro and not
a function call, i mean "for_each_line (bla)";
i only recently adopted that space into my coding style, imho it's a sane
convention.)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/2185e18c_006e352c?usp… :
PS4, Line 208: case '\0':
(hmm i thought it was '\n\n' or '\r\n\r\n' delimiting the SDP body ... ah
right, we pre-parse that and overwrite with '\0'? anyway, it's from before
this patch.)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/33d1333a_9b058023?usp… :
PS4, Line 210: goto mgcp_header_done;
'return 0;'
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/654c4891_061274ff?usp… :
PS4, Line 212: LOGP(DLMGCP, LOGL_NOTICE, "CRCX: unhandled option:
'%c'/%d\n", *line, *line);
(technically the caller should log instead of this function, based on the returned 539)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/dd2467a1_da56e762?usp… :
PS4, Line 218: return 0;
have i ever seen a useless label =)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/bd653b5d_b369ab6f?usp… :
PS4, Line 257: LOGP(DLMGCP, LOGL_NOTICE, "wrong MGCP option format:
'%s'\n", line);
context-less logging is so 2009
patch removes the logging context: endp / trunk.
it's of course always a pain to add logging context crossing layers, thinking of
error_callback functions or a string returned or somesuch...
i feel the reasons (what does MGCP parsing code care where the data fell out), but it
would be very nice, in principle from API design, to allow logging context on MGCP parsing
results -- rather adding more ctx than dropping existing ctx.
this aspect is fairly important to me, it impacts sysadmin operations and basic usability
of osmo-mgw in presence of errors.
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/e3a27bd3_cbe1df1f?usp… :
PS4, Line 710: if (osmux_init(trunk) < 0) {
drops logging context: endp reduced to just trunk.
makes sense from POV that this setup is done once per trunk.
but it is also nice to know which subscriber / which endp caused it.
(otherwise i would favor adding the LOGP into osmux_init() itself / have a
osmux_init_if_needed() in the main API.)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/1af60a1c_de00feaf?usp… :
PS4, Line 846: case -523:
(i humbly favor passing the MGCP result codes as positive numbers, more like a result code
enum, and less like -errno "nonsense". After all they are 1:1 sent out on the
wire, and there seems no good reason for passing them negative?)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/3803b41b_59321197?usp… :
PS4, Line 853: return create_err_response(rq->trunk, NULL, -rc, "CRCX",
pdata->trans);
(hmm, no counter for this error path, probably an oversight by whichever patch added the
counters. that's cool about this patch: now this omission shows clear as day, and was
not readily visible in the earlier code)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/93487c11_6d9bb1a5?usp… :
PS4, Line 873: hpars->remote_osmux_cid = MGCP_PARSE_HDR_PARS_OSMUX_CID_UNSET;
what i don't like here semantically: hpars is the pristine result of parsing some
received PDU and should be immutable. By modifying it here, it is tainted. The flag to
skip osmux when it is not enabled should live outside of the parsing result; so that in
hypothetical logging and future vty dump code, we can still see that the peer did actually
ask for osmux, and did not get it for other reasons.
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/12d1d027_a2e0a2da?usp… :
PS4, Line 898: /* Update endp->x_osmo_ign: */
(lol funny comment)
https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/e1609b96_e3f3b72f?usp… :
PS4, Line 899: endp->x_osmo_ign |= hpars->x_osmo_ign;
are you sure about `|=` instead of `=`???
how does one disable an X-Osmo-Ign once it is enabled? impossible?
--
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: 4
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: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 15 Jan 2025 16:52:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No