<p><b>@henningw</b> commented on this pull request.</p>
<p>I have added several small comments about places that I noticed, but as this is a large and complex module, more time is needed. Some general remarks:</p>
<ul>
<li>
<p>i noticed some differences in several module parts with regards to documentation and error handling. Some parts are quite extensive, and some parts lacks some checks. This are probably parts that are still changing - just an observation</p>
</li>
<li>
<p>please check the return status consistently for functions (i could imagine that the external library functions like init(), destroy(), create_decoder(), attach() etc.. can fail)</p>
</li>
<li>
<p>Replace OpenSER with Kamailio in the comments</p>
</li>
<li>
<p>remove the ~150kb music file, maybe there is the possibility to add a link in the docs instead to a source</p>
</li>
<li>
<p>all functions and variables that are not needed in a global scope (e.g. because they are accessed from other modules or cfg script) should be made static, to restrict their visibility to the module compilation unit. Variables can of course also moved to the respective functions, this is even better.</p>
</li>
</ul>
<p>I have made more comments at several places directly in the code.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230203516">src/modules/rtp_media_server/rms_media.c</a>:</p>
<pre style='color:#555'>> + ms_connection_helper_start(&h);
+ ms_connection_helper_link(&h, m1->ms_rtprecv, -1, 0);
+ ms_connection_helper_link(&h, m2->ms_rtpsend, 0, -1);
+
+ // direction 2
+ ms_connection_helper_start(&h);
+ ms_connection_helper_link(&h, m2->ms_rtprecv, -1, 0);
+ ms_connection_helper_link(&h, m1->ms_rtpsend, 0, -1);
+
+ ms_ticker_attach_multiple(
+ m1->ms_ticker, m1->ms_rtprecv, m2->ms_rtprecv, NULL);
+
+ return 1;
+}
+
+#define MS_UNUSED(x) ((void)(x))
</pre>
<p>What is the purpose of this define?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230203824">src/modules/rtp_media_server/rms_media.c</a>:</p>
<pre style='color:#555'>> + }
+ ms_factory_init_plugins(f);
+ ms_factory_enable_statistics(f, TRUE);
+ ms_factory_reset_statistics(f);
+ return f;
+}
+
+int rms_media_init()
+{
+ OrtpMemoryFunctions ortp_memory_functions;
+ ortp_memory_functions.malloc_fun = ptr_shm_malloc;
+ ortp_memory_functions.realloc_fun = ptr_shm_realloc;
+ ortp_memory_functions.free_fun = ptr_shm_free;
+ ortp_set_memory_functions(&ortp_memory_functions);
+ ortp_init();
+ vars = ortp_malloc(sizeof(shared_global_vars_t));
</pre>
<p>Why you use the ortp_malloc() for allocation of Kamailio module structures?<br>
The malloc can also fail.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230204178">src/modules/rtp_media_server/rms_media.h</a>:</p>
<pre style='color:#555'>> +#define rms_media_h
+
+#include "../../core/mem/shm.h"
+#include "mediastreamer2/mediastream.h"
+#include "mediastreamer2/msrtp.h"
+#include "mediastreamer2/mediastream.h"
+#include "mediastreamer2/dtmfgen.h"
+#include "mediastreamer2/msfileplayer.h"
+#include "mediastreamer2/msfilerec.h"
+#include "mediastreamer2/msrtp.h"
+#include "mediastreamer2/mstonedetector.h"
+#include "mediastreamer2/msfilter.h"
+//#include <mediastreamer2/mediastream.h>
+#include <ortp/ortp.h>
+#include <ortp/port.h>
+#define MS_UNUSED(x) ((void)(x))
</pre>
<p>see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230204384">src/modules/rtp_media_server/rms_sdp.c</a>:</p>
<pre style='color:#555'>> +
+// https://tools.ietf.org/html/rfc4566
+// (protocol version)
+const char *sdp_v = "v=0\r\n";
+// (session name)
+const char *sdp_s = "s=-\r\n";
+// (time the session is active)
+const char *sdp_t = "t=0 0\r\n";
+//"a=rtpmap:101 telephone-event/8000\r\n"
+//"a=fmtp:101 0-15\r\n";
+//"a=rtpmap:0 PCMU/8000\r\n"
+//"a=rtpmap:8 PCMA/8000\r\n"
+//"a=rtpmap:96 opus/48000/2\r\n"
+//"a=fmtp:96 useinbandfec=1\r\n";
+
+static char *rms_shm_strdup(char *source)
</pre>
<p>Can't you use the core shm_str_dup() instead?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230204786">src/modules/rtp_media_server/rms_sdp.c</a>:</p>
<pre style='color:#555'>> + char sdp_o[128];
+ snprintf(
+ sdp_o, 128, "o=- 1028316687 1 IN IP4 %s\r\n", sdp_info->local_ip.s);
+ body->len += strlen(sdp_o);
+
+ // (connection information -- not required if included in all media)
+ char sdp_c[128];
+ snprintf(sdp_c, 128, "c=IN IP4 %s\r\n", sdp_info->local_ip.s);
+ body->len += strlen(sdp_c);
+
+ char sdp_m[128];
+ snprintf(sdp_m, 128, "m=audio %d RTP/AVP %d\r\n", sdp_info->udp_local_port,
+ payload_type_number);
+ body->len += strlen(sdp_m);
+
+ body->s = pkg_malloc(body->len + 1);
</pre>
<p>pkg_malloc can fail</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230206087">src/modules/rtp_media_server/rtp_media_server.c</a>:</p>
<pre style='color:#555'>> +{
+ if(rank == PROC_MAIN) {
+ int pid;
+ pid = fork_process(PROC_XWORKER, "RTP_media_server", 1);
+ if(pid < 0)
+ return -1;
+ if(pid == 0) {
+ rms_session_manage_loop();
+ return 0;
+ }
+ }
+ int rtn = 0;
+ return (rtn);
+}
+
+int rms_str_dup(str *dst, str *src, int shared)
</pre>
<p>please use the pkg_str_dup() and shm_str_dup() in the core</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230206971">src/modules/rtp_media_server/rtp_media_server.c</a>:</p>
<pre style='color:#555'>> + return 1;
+error:
+ rms_sdp_info_free(sdp_info);
+ return 0;
+}
+
+static int rms_relay_call(struct sip_msg *msg)
+{
+ if(!tmb.t_relay(msg, NULL, NULL)) {
+ LM_INFO("t_ralay error\n");
+ return -1;
+ }
+ return 1;
+}
+
+str reply_headers = {0, 0};
</pre>
<p>unused</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230207166">src/modules/rtp_media_server/rtp_media_server.c</a>:</p>
<pre style='color:#555'>> +error:
+ rms_sdp_info_free(sdp_info);
+ return 0;
+}
+
+static int rms_relay_call(struct sip_msg *msg)
+{
+ if(!tmb.t_relay(msg, NULL, NULL)) {
+ LM_INFO("t_ralay error\n");
+ return -1;
+ }
+ return 1;
+}
+
+str reply_headers = {0, 0};
+str headers = str_init("Max-Forwards: 70" CRLF);
</pre>
<p>This and the following vars are only used in one functions. Please consider moving them to the respective functions, they don't need to be globally visible.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1701#discussion_r230207673">src/modules/rtp_media_server/rms_sdp.c</a>:</p>
<pre style='color:#555'>> +const char *sdp_s = "s=-\r\n";
+// (time the session is active)
+const char *sdp_t = "t=0 0\r\n";
+//"a=rtpmap:101 telephone-event/8000\r\n"
+//"a=fmtp:101 0-15\r\n";
+//"a=rtpmap:0 PCMU/8000\r\n"
+//"a=rtpmap:8 PCMA/8000\r\n"
+//"a=rtpmap:96 opus/48000/2\r\n"
+//"a=fmtp:96 useinbandfec=1\r\n";
+
+static char *rms_shm_strdup(char *source)
+{
+ char *copy;
+ if(!source)
+ return NULL;
+ copy = (char *)ortp_malloc(strlen(source) + 1);
</pre>
<p>see other comment</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/kamailio/kamailio/pull/1701#pullrequestreview-170889197">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AF36ZTHHXXgoIp2f5Dm9-W42qKwkBq8wks5uq25GgaJpZM4YF7gV">mute the thread</a>.<img src="https://github.com/notifications/beacon/AF36ZSgxIs4gUkhjIlDWgpx362xPm50qks5uq25GgaJpZM4YF7gV.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@henningw commented on #1701"}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1701#pullrequestreview-170889197"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/kamailio/kamailio/pull/1701#pullrequestreview-170889197",
"url": "https://github.com/kamailio/kamailio/pull/1701#pullrequestreview-170889197",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@henningw commented on 1701",
"sections": [
{
"text": "I have added several small comments about places that I noticed, but as this is a large and complex module, more time is needed. Some general remarks:\r\n\r\n- i noticed some differences in several module parts with regards to documentation and error handling. Some parts are quite extensive, and some parts lacks some checks. This are probably parts that are still changing - just an observation\r\n\r\n- please check the return status consistently for functions (i could imagine that the external library functions like init(), destroy(), create_decoder(), attach() etc.. can fail)\r\n- Replace OpenSER with Kamailio in the comments\r\n- remove the ~150kb music file, maybe there is the possibility to add a link in the docs instead to a source\r\n- all functions and variables that are not needed in a global scope (e.g. because they are accessed from other modules or cfg script) should be made static, to restrict their visibility to the module compilation unit. Variables can of course also moved to the respective functions, this is even better.\r\n\r\nI have made more comments at several places directly in the code.",
"activityTitle": "**Henning Westerholt**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@henningw",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/kamailio/kamailio/pull/1701#pullrequestreview-170889197"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 404207637\n}"
}
],
"themeColor": "26292E"
}
]</script>