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=email : PS4, Line 7: #define MGCP_PARSE_HDR_PARS_OSMUX_CID_WILDCARD (-1) > (IMHO enums are favorable to #defines) 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=email : PS4, Line 14: bool have_sdp; > (i'd have called it 'sdp_present' -- 'have' sounds to me like requesting to > generate something, not […] 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=email : PS4, Line 27: .x_osmo_ign = 0, > (the " = 0" values are also there implicitly, so it could be just the two > .mode and . […] 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=email : PS4, Line 34: char *save; > (naming: every struct member saves values -- so save what? maybe > mgcp_msg_str?) 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=email : PS4, Line 38: /* MGCP Body: */ > naming: the "body" in MGCP is the SDP message string starting with a blank > line, so "MGCP Body: mgcp […] Done File src/libosmo-mgcp/mgcp_msg.c: https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/c5b4520b_83a97789?usp=email : PS4, Line 170: ea > messeage Done https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/9a20d439_96bf5855?usp=email : PS4, Line 170: ha > haders gonna hade Done https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/1da64109_a64a70ab?usp=email : PS4, 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 […] 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=email : PS4, Line 179: /* parse CallID C: and LocalParameters L: */ > (i guess this comment can go, no useful information conveyed and not even > accurate. […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/7841886f_c1a5e7fd?usp=email : PS4, Line 180: e( > (i think the linter now usually asks for a space when it is a for-loop style > macro and not a functio […] 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=email : PS4, Line 208: case '\0': > (hmm i thought it was '\n\n' or '\r\n\r\n' delimiting the SDP body ... […] No idea, simply moving old code here. https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/b5afe5fb_7f9ddf7c?usp=email : PS4, Line 210: goto mgcp_header_done; > 'return 0;' 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=email : PS4, 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) 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=email : PS4, Line 218: return 0; > have i ever seen a useless label =) 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=email : PS4, 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 s […] 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=email : PS4, Line 898: /* Update endp->x_osmo_ign: */ > (lol funny comment) 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 <[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:21:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]>
