Attention is currently required from: laforge, neels. pespin 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: (3 comments) File src/libosmo-mgcp/mgcp_msg.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/3dca0fc8_739ced20?usp=email : PS4, Line 257: LOGP(DLMGCP, LOGL_NOTICE, "wrong MGCP option format: '%s'\n", line); > context-less logging is so 2009 […] An error like this should be catched up by caller layers holding conn information, and then print context there. Otherwise maintaining all this code is a nightmare. Who is actually calling this? who we want to log? a endp? a conn? a trunk? a bird? a spacecraft? This API is really to low level to actually have to care about that. File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/e8d53e7e_f7fb186d?usp=email : PS4, Line 710: if (osmux_init(trunk) < 0) { > drops logging context: endp reduced to just trunk. […] if this is called due to whatever endp/conn triggerer and this function fails, then it's up to the caller to log whatever context it has upon osmux initialization failure. Again, there's too many contexts which may be of interest here depending on the code path, so let's keep it at the caller wherever it is interesting. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/7808be8e_430d817f?usp=email : PS4, Line 899: endp->x_osmo_ign |= hpars->x_osmo_ign; > are you sure about `|=` instead of `=`??? […] Yes |= because it's in the endp, it's shared by several conns. Otherwise a 2nd conn entering would remove prior information there. If you look at the previous code you'll see it also does |=. Whether it is clean up on release or similar I don't know, but I'm not modifying the logic here. This x_osmo_sign will probably need more cleanup in the future, but I cannot do it here, I simply stick to what we had. -- 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Comment-Date: Wed, 15 Jan 2025 17:30:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]>
