[L] Change in osmo-mgw[master]: mgw: CRCX: Split mgcp header pars parsing into a previous step

Attention is currently required from: daniel, 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 5: (5 comments) File include/osmocom/mgcp/mgcp_protocol.h: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/219b8149_7ba9106c?usp… : PS4, Line 7: #define MGCP_PARSE_HDR_PARS_OSMUX_CID_WILDCARD (-1) i disagree with that position. i feel that lists of separate #defines for any number of related ints, nevermind the used range, is a hack from times where our early ancestors didn't know about curly braces yet and had barely discovered fire ... enums are an actual part of the C language proper that can be used as return type or argument type to clarify things, #defines are a preprocessor shim, much like a flea circus... i will not block this, but i challenge you to admit that mentioning the int range is just a lame excuse. File src/libosmo-mgcp/mgcp_msg.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/df8aab49_10bc8aff?usp… : PS4, Line 210: goto mgcp_header_done; It is a label with a single caller, that simply does "return 0": there is no logic to preserve. It is the textbook example of meaningless bloat. it is a non-functional cosmetic issue, so i am dialing back a bit, but i find this beyond weird. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/e66fc108_5abbf072?usp… : PS4, Line 218: return 0; empty words. (resolving this one so we don't say the same things twice) File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/57f448d3_6904a35a?usp… : PS4, Line 873: hpars->remote_osmux_cid = MGCP_PARSE_HDR_PARS_OSMUX_CID_UNSET; This patch does exactly the right thing: it introduces proper layering boundaries. So I would like the new code to observe the boundaries that it introduces. This is not about a fix of earlier code. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/90590321_9523a011?usp… : PS4, Line 899: endp->x_osmo_ign |= hpars->x_osmo_ign; ok! -- 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: 5 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-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Thu, 16 Jan 2025 11:55:23 +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>