<p><b>@rfuchs</b> requested changes on this pull request.</p>
<p>Couple of suggestions on how to improve the flow of logic here. You can leave them unchanged if you prefer, then I will address them myself afterwards.</p>
<p>However, please do document these changes in <code>doc/rtpengine_admin.xml</code>, perhaps with an explanation of how they might be useful.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1103#discussion_r113939829">src/modules/rtpengine/rtpengine.c</a>:</p>
<pre style='color:#555'>> @@ -1784,6 +1784,8 @@ static int parse_flags(struct ng_flags_parse *ng_flags, struct sip_msg *msg, enu
case 6:
if (str_eq(&key, "to-tag")) {
ng_flags->to = 1;
+ if (val.s && val.len > 0)
</pre>
<p>If a value is given, you could simply leave <code>ng_flags->to</code> alone (as zero) and <code>goto generic</code></p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/1103#discussion_r113943338">src/modules/rtpengine/rtpengine.c</a>:</p>
<pre style='color:#555'>> @@ -1967,8 +1970,13 @@ static bencode_item_t *rtpp_function_call(bencode_buffer_t *bencbuf, struct sip_
if (ng_flags.rtcp_mux && ng_flags.rtcp_mux->child)
bencode_dictionary_add(ng_flags.dict, "rtcp-mux", ng_flags.rtcp_mux);
- bencode_dictionary_add_str(ng_flags.dict, "call-id", &callid);
-
+ temp.s = NULL;
+ if (!bencode_dictionary_get_str(ng_flags.dict, "call-id", &temp))
</pre>
<p>You can get rid of the temp variable and the case distinction below by simply retrieving the given call ID into <code>callid</code>. Otherwise, at the very least please rename <code>temp</code> to something more meaningful, as there's quite a long distance between this and the place where it's actually being used.</p>
<p>Generally though, instead of doing a <code>_get_str</code> here, I would prefer <code>parse_flags</code> to take care of this and update a flag in <code>ng_flags</code> to signal that a call ID is already present. (Reason is that a dictionary lookup for a constructed dictionary reverts to a linear search, and I cringe at linear searches. <g-emoji alias="smile" fallback-src="https://assets-cdn.github.com/images/icons/emoji/unicode/1f604.png" ios-version="6.0">😄</g-emoji> ) The same applies to the fromtag/totag handling below, which can be solved in a similar way.</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/1103#pullrequestreview-35371108">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AF36ZQXaY7RBHhaKtrNTx_D8MXEOBFU-ks5r0frOgaJpZM4NLScH">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AF36ZTK6JyY8f80fMTfYRcIRIoBKnuKBks5r0frOgaJpZM4NLScH.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/kamailio/kamailio/pull/1103#pullrequestreview-35371108"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>
<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://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@rfuchs requested changes on #1103"}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1103#pullrequestreview-35371108"}}}</script>