Attention is currently required from: daniel, laforge, pespin.

neels 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 5: Code-Review-1

(11 comments)

Patchset:

PS5: my -1 point is removing of logging context that was present before this patch.

File include/osmocom/mgcp/mgcp_protocol.h:

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/ea1670fe_385aa8c5?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.

a response of "i don't care" for your own naming choices usually goes with accepting CR suggestions...?

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/924dde05_026f7e64?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 […]

ack, grep is a good point

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/5cdd2768_71ccbf6d?usp=... : PS4, 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:

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/b5717a7b_03529081?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.

ack

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/2814809a_32f62a4d?usp=... : PS4, Line 180: e(

There was a discussion about this recently iirc. Currently we have: […]

ack, i will not bother

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/8d2efa47_7d42e29e?usp=... : PS4, Line 208: case '\0':

No idea, simply moving old code here.

ack

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/871f1297_92134113?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.

ack

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/813d32fa_2d5fe38d?usp=... : PS4, 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:

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/6b03f072_23137015?usp=... : PS4, 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!

https://gerrit.osmocom.org/c/osmo-mgw/+/39224/comment/8ac1ecd5_f1ae0d62?usp=... : PS4, 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?)