osmo-mgw[master]: libosmo-mgcp: Connection Identifiers are allocated by MGW, n...

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Mon Nov 27 13:10:03 UTC 2017


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 <laforge at gnumonks.org>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list