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
Patch Set 1:
(11 comments)
https://gerrit.osmocom.org/#/c/4146/1/include/osmocom/mgcp_client/mgcp_client.h
File include/osmocom/mgcp_client/mgcp_client.h:
PS1, Line 49: MSG_T
> msg_t typically referst to "message type", which it isn't. What about MGCP
(I like MGCP_MSG_PRESENCE_*)
Line 60: uint32_t presence;
(and a comment here "see MGCP_MSG_PRESENCE_*")
Line 61: char endpoint[MGCP_EINDPOINT_MAXLEN];
"EIND" -> "END"
Line 64: struct sockaddr_storage rtp_addr;
Would it be less complex to store the address as a string? Then we can leave sockaddr complexities out of message composition / parsing, sticking to plain string tokens ... would this bring up other problems?
Line 97: enum mgcp_connection_mode mode);
void foo(...) OSMO_DEPRECATED("Use bar() instead");
https://gerrit.osmocom.org/#/c/4146/1/src/libosmo-mgcp-client/mgcp_client.c
File src/libosmo-mgcp-client/mgcp_client.c:
Line 631: static char compose[4096 - 128];
> where do these magic numbers come from? how are we sure we don't overflow t
4kb less headroom, right? :)
Line 662: return NULL;
log an error?
Line 665: /* Check if mandatory fiels are missing */
("fields")
Line 667: return NULL;
log an error?
Line 671: sprintf(compose + strlen(compose), " %s", mgcp_msg->endpoint);
> so we have a long series of "sprintf(compose + strlen(compose)" statements
I agree that we need more safety here. The implementation is not trivial.
I agree that a kind of msgb_printf() utility function would be useful to cover everything generically.
A difficulty though is that e.g. for
printf("C: %x\r\n", call_id)
we don't know in advance how much length to request from msgb_put().
A way would be to use snprintf, limiting printed chars by the size parameter, and using the return val of snprintf, to obtain the nr of chars written.
i.e. obtain from the msgb the remaining tail room and where the next string goes, feed those to snprintf(), then tell msgb_put() the returned length after snprintf() is completed.
We also need to detect a situation where we have hit the end of the buffer (when the len returned by snprintf() is > the remaining buffer length), log an error and return NULL. Every time we append to the buf.
https://gerrit.osmocom.org/#/c/4146/1/tests/mgcp_client/mgcp_client_test.c
File tests/mgcp_client/mgcp_client_test.c:
Line 213: msgb_free(msg);
(also add tests that hit the buffer size limits)
--
To view, visit https://gerrit.osmocom.org/4146
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I29c5e2fb972896faeb771ba040f015592487fcbe
Gerrit-PatchSet: 1
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes