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=email : PS4, Line 7: #define MGCP_PARSE_HDR_PARS_OSMUX_CID_WILDCARD (-1) > Yeah in general, but in this case it's not a closed-set of options, but 2 > specific options within in […] 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=email : PS4, Line 210: goto mgcp_header_done; > I prefer handling it below as it is now, and as it used to be too. […] 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=email : PS4, Line 218: return 0; > To me this makes it easier to understand that you can finish properly based > on whether you arrived t […] 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=email : PS4, Line 873: hpars->remote_osmux_cid = MGCP_PARSE_HDR_PARS_OSMUX_CID_UNSET; > Yeah well a lot of stuff is quite fucked up, but we cannot fix it all in one > commit. […] 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=email : PS4, Line 899: endp->x_osmo_ign |= hpars->x_osmo_ign; > Yes |= because it's in the endp, it's shared by several conns. […] 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Comment-Date: Thu, 16 Jan 2025 11:55:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: pespin <[email protected]>
