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: (16 comments) File include/osmocom/mgcp/mgcp_protocol.h: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/13020a02_f0734010?usp… : 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 int range, so it's fine this way. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/9f84f7e9_d24bcaab?usp… : PS4, Line 14: bool have_sdp; I don't care tbh, feel free to submit a patch renaming when you do further work on related code. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/530bba98_68772968?usp… : PS4, Line 27: .x_osmo_ign = 0, I still prefer having them here explicitly since it provides a quick look (also through grep) on the default value. It shouldn't really matter performance wise anyway. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/48faf694_e9e00a49?usp… : PS4, Line 34: char *save; I'm simply keeping the previously existing var name. Feel free to rename it once everything is merged, will be far easier since previous code base is a mess. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/ee645a74_08e25ecf?usp… : PS4, Line 38: /* MGCP Body: */ Done File src/libosmo-mgcp/mgcp_msg.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/c5b4520b_83a97789?usp… : PS4, Line 170: ea Done https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/9a20d439_96bf5855?usp… : PS4, Line 170: ha Done https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/1da64109_a64a70ab?usp… : PS4, Line 173: int mgcp_parse_hdr_pars(struct mgcp_parse_data *pdata) That's a internal static library of osmo-mgw, so no need for that. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/4332aa3d_77f33e55?usp… : PS4, Line 179: /* parse CallID C: and LocalParameters L: */ Done https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/7841886f_c1a5e7fd?usp… : PS4, Line 180: e( There was a discussion about this recently iirc. Currently we have: $ ag "for_each\(" | wc -l 4699 In any case, this for_each_line() macro is used 11 times, all the times this same way as it is now. Feel free to submit a follow up patch later on changing it for all the use cases. I'm simply keeping the existing syntax here. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/ff2d81d7_fc34b6bf?usp… : PS4, Line 208: case '\0': No idea, simply moving old code here. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/b5afe5fb_7f9ddf7c?usp… : PS4, Line 210: goto mgcp_header_done; I prefer handling it below as it is now, and as it used to be too. I don't want to change the existing logic here. Feel free to do so in a follow-up patch if you really think improves stuff, but see my comment below. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/b4d02310_e9999871?usp… : PS4, Line 212: LOGP(DLMGCP, LOGL_NOTICE, "CRCX: unhandled option: '%c'/%d\n", *line, *line); It is further updated in follow-up patch, I tried to do it in steps. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/c1ff4ab8_d7afb850?usp… : PS4, Line 218: return 0; To me this makes it easier to understand that you can finish properly based on whether you arrived to the end of the string, or because you arrived to the end of the header and SDP is coming. I don't plan to change this for now. File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/b7782d9a_7ae24da9?usp… : 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. Specially all the osmux stuff. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/3e355216_df04f49d?usp… : PS4, Line 898: /* Update endp->x_osmo_ign: */ All this x_osmo_ign will also probably be improved at a later point. -- 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: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Wed, 15 Jan 2025 17:21:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>