Patch Set 6: Code-Review-1 (10 comments)
nice, but found some details, mostly in the test https://gerrit.osmocom.org/#/c/4905/6/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: Line 487: ENDPOINT_NUMBER(endp), *line, *line); the commit log says, we should reject MGCP requests that contain 'I:', this is logging the error only. I guess this is ok, would just be nice to have commit log in line with the implementation. https://gerrit.osmocom.org/#/c/4905/6/tests/mgcp/mgcp_test.c File tests/mgcp/mgcp_test.c: Line 490: len = sprintf((char *)msg->data, "%s", str); (curious, in one sprintf() the str directly serves as format, in the other we use "%s" and str as parameter. Seems to me that this could have used str as format directly all the time (even before this patch).) Line 574: if (conn_id[i] == '\n' || conn_id[i] == '\n') -1: did you mean '\r' instead of \n twice? Line 766: char conn_id[256]; (what is this for, just a temp buffer while reading the conn_id? Seems it is always just copied straight to last_conn_id, right? Why not use only last_conn_id?) Line 767: char last_conn_id[256]; -1: IIUC last_conn_id may be fed into create_msg as uninitialized mem and gets printed into the message? ... ah, a previous iteration should have written to it. But let's start off with it saying "invalid" or something. Line 784: inp = create_msg(t->req, last_conn_id); <- uninitialized here Line 796: memcpy(last_conn_id, conn_id, sizeof(conn_id)); <- changes here Line 802: inp = create_msg(t->req, last_conn_id); -1: at this point the last_conn_id may have changed, right? Not a clean re-transmission? https://gerrit.osmocom.org/#/c/4905/6/tests/mgcp/mgcp_test.ok File tests/mgcp/mgcp_test.ok: Line 24: Response matches our expectations. (are these three lines useful output?) Line 89: using message with patched conn_id for comparison -1: I'd prefer to see the actual patched message instead of %s and a loose fact that it was patched, so that we can spot whether all %s were in fact fed with data correctly. -- To view, visit https://gerrit.osmocom.org/4905 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iab6a6038e7610c62f34e642cd49c93d11151252c Gerrit-PatchSet: 6 Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Owner: Harald Welte <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: dexter <[email protected]> Gerrit-HasComments: Yes
