Attention is currently required from: laforge, neels.
16 comments:
File include/osmocom/mgcp/mgcp_protocol.h:
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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:
messeage
Done
haders gonna hade
Done
Patch Set #4, 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.
Patch Set #4, Line 179: /* parse CallID C: and LocalParameters L: */
(i guess this comment can go, no useful information conveyed and not even accurate. […]
Done
(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
4699In 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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:
Patch Set #4, 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.
Patch Set #4, 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 change 39224. To unsubscribe, or for help writing mail filters, visit settings.