Attention is currently required from: laforge, pespin.
23 comments:
Patchset:
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:
Patch Set #4, Line 7: #define MGCP_PARSE_HDR_PARS_OSMUX_CID_WILDCARD (-1)
(IMHO enums are favorable to #defines)
Patch Set #4, 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.)
Patch Set #4, 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)
Patch Set #4, Line 34: char *save;
(naming: every struct member saves values -- so save what? maybe mgcp_msg_str?)
Patch Set #4, 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:
(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.
haders gonna hade
messeage
Patch Set #4, 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?
Patch Set #4, Line 179: /* parse CallID C: and LocalParameters L: */
(i guess this comment can go, no useful information conveyed and not even accurate.)
(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.)
Patch Set #4, 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.)
Patch Set #4, Line 210: goto mgcp_header_done;
'return 0;'
Patch Set #4, 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)
Patch Set #4, Line 218: return 0;
have i ever seen a useless label =)
Patch Set #4, 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:
Patch Set #4, 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.)
Patch Set #4, 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?)
Patch Set #4, 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)
Patch Set #4, 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.
Patch Set #4, Line 898: /* Update endp->x_osmo_ign: */
(lol funny comment)
Patch Set #4, 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 change 39224. To unsubscribe, or for help writing mail filters, visit settings.