Attention is currently required from: daniel, laforge, pespin.
Patch set 5:Code-Review -1
11 comments:
Patchset:
my -1 point is removing of logging context that was present before this patch.
File include/osmocom/mgcp/mgcp_protocol.h:
Patch Set #4, 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.
a response of "i don't care" for your own naming choices usually goes with accepting CR suggestions...?
Patch Set #4, Line 27: .x_osmo_ign = 0,
I still prefer having them here explicitly since it provides a quick look (also through grep) on the […]
ack, grep is a good point
Patch Set #4, Line 34: char *save;
I'm simply keeping the previously existing var name. […]
hmm, but you are taking local variables and putting them in a struct, so this is a good time to choose names?
i agree that avoiding renames makes the patch diff easier to read.
Would be great to have useful names in the end.
File src/libosmo-mgcp/mgcp_msg.c:
Patch Set #4, 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.
ack
There was a discussion about this recently iirc. Currently we have: […]
ack, i will not bother
Patch Set #4, Line 208: case '\0':
No idea, simply moving old code here.
ack
Patch Set #4, 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.
ack
Patch Set #4, Line 257: LOGP(DLMGCP, LOGL_NOTICE, "wrong MGCP option format: '%s'\n", line);
I submitted https://gerrit.osmocom. […]
What i would like to see is that the parsing code returns error messages to the caller. Then the caller should log the error message with the relevant context added.
What i don't want to see is that the parsing code does a lot of context-less logging of errors, and the user needs to guess from adjacent logging where the error originated from.
(The linked patch also does context-less logging)
I will veto removing logging context that was present before this patch.
File src/libosmo-mgcp/mgcp_protocol.c:
Patch Set #4, Line 710: if (osmux_init(trunk) < 0) {
Comment removed by: pespin; Reason: wrong thread
I agree that the endp is only marginally related and could be omitted from logging context.
But then the LOGPTRUNK() below should be done within osmux_init().
thanks!
Patch Set #4, Line 846: case -523:
(i humbly favor passing the MGCP result codes as positive numbers, more like a result code enum, and […]
(you don't agree?)
To view, visit change 39224. To unsubscribe, or for help writing mail filters, visit settings.