<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2971/">https://reviewboard.asterisk.org/r/2971/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 31st, 2013, 6:48 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Most issues I ran across in my review are memory leaks in off-nominal paths.
I noted a couple of usability concerns while looking over this review.
1) Under this implementation If a user has menu options set via the CONFBRIDGE() function, then when that user enters a ConfBridge(), any specified menu profile name passed as an application argument is ignored in favor of the menu options that were set via CONFBRIDGE().
2) If a user who has set menu options using CONFBRIDGE() is placed into one ConfBridge(), leaves, and enters a second ConfBridge(), the menu options get applied to both invocations of the application.
The two kind of relate to each other because the essential problem at the moment is that once a user is given menu options via CONFBRIDGE(), the privilege cannot be revoked. One way around this would be to only apply the CONFBRIDGE()-created menu options if no menu profile name is provided to ConfBridge(). This way, you can interpret an explicit menu name to mean that the user wants that menu, as configured, to be the menu to use for ConfBridge. A second way would be to require a special name to be passed to ConfBridge() in order to use the menu options set via CONFBRIDGE(). No provided name would use the default menu profile. A third way would be to remove the menu options from the channel's datastore when the channel is removed from a ConfBridge(). This would mean that any user-specific menu options would need to be re-applied in the dialplan prior to a second invocation of ConfBridge(). This third option would only solve problem number 2 and not problem number 1, though.
I'm partial to the first suggestion, myself. What do you think?</pre>
</blockquote>
<p>On October 31st, 2013, 7:10 p.m. UTC, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This concern also applies to the bridge and user profiles. It might be better to have a CONFBRIDGE(menu,wipe) value as a companion to the CONFBRIDGE(menu,template). Also add similar for the bridge and user profiles.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I did it this way (where the datastore overrides application options) deliberately because this is how the dynamic confbridge and dynamic user settings work as well. Granted... this may have not been deliberate. It looks like the documentation suggests the CONFBRIDGE profiles should only be used when the application arguments are left blank. I'll go ahead and fix that for all of them.
As for problem 2... which would only be slightly addressed by having the dynamic profiles applied only when application arguments aren't provided instead... I think the best way to address that would be to have a way to clear existing settings with the CONFBRIDGE function. I'll see if I can't come up with something.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 31st, 2013, 6:48 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2971/diff/3/?file=47833#file47833line2248" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/apps/confbridge/conf_config_parser.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int conf_find_menu_entry_by_sequence(const char *dtmf_sequence, struct conf_menu *menu, struct conf_menu_entry *result)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2157</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_lock</span><span class="p">(</span><span class="n">menu</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2221</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ao2_lock</span><span class="p">(</span><span class="n">menu</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">To prevent you from having to unlock the menu at each exit point of this function, you could instead use a SCOPED_AO2LOCK() here.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 31st, 2013, 6:48 p.m. UTC, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2971/diff/3/?file=47833#file47833line2258" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/apps/confbridge/conf_config_parser.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int conf_find_menu_entry_by_sequence(const char *dtmf_sequence, struct conf_menu *menu, struct conf_menu_entry *result)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2166</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">copy_menu_entry</span><span class="p">(</span><span class="o">&</span><span class="n">pvt</span><span class="o">-></span><span class="n">menu_entry</span><span class="p">,</span> <span class="n">menu_entry</span><span class="p">))</span> <span class="p">{</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2230</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">copy_menu_entry</span><span class="p">(</span><span class="o">&</span><span class="n">pvt</span><span class="o">-></span><span class="n">menu_entry</span><span class="p">,</span> <span class="n">menu_entry</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2167</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ast_free</span><span class="p">(</span><span class="n">pvt</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2231</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ast_free</span><span class="p">(</span><span class="n">pvt</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2168</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ao2_unlock</span><span class="p">(</span><span class="n">menu</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2232</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ao2_unlock</span><span class="p">(</span><span class="n">menu</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2169</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">ao2_ref</span><span class="p">(</span><span class="n">menu</span><span class="p">,</span> <span class="o">-</span><span class="mi">1</span><span class="p">);</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2170</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">return</span> <span class="o">-</span><span class="mi">1</span><span class="p">;</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2233</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="k">return</span> <span class="o">-</span><span class="mi">1</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">2171</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="p">}</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2234</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Calling ast_free(pvt) here isn't enough to ensure that memory is properly cleaned up. The list items that were copied onto &pvt->menu_entry need to be freed as well.
(FYI, I recognize this wasn't your doing, but I noticed it anyway since I was in the area)</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Threw in a conf_menu_entry_destroy(&pvt->menu_entry) to cover it. I think that'll take care of it anyway.</pre>
<br />
<p>- jrose</p>
<br />
<p>On October 31st, 2013, 4:14 p.m. UTC, jrose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Matt Jordan, Mark Michelson, and rmudgett.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Oct. 31, 2013, 4:14 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22760">ASTERISK-22760</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Adds the ability to dynamically create menus using the CONFBRIDGE dialplan function. This includes the ability to use the existing options as well as to specify templates with the 'template' option.
I suppose it's worth mentioning that there is one minor bit of awkwardness. Both the channel datastore and the conference user will end up with an allocation of the menu. This is probably for the best for a couple reasons though. For instance, the CONFBRIDGE function can modify the contents of the datastore allocated menu while the user is still in the conference if it is set via AMI. We probably don't want the menu to be modified in place during this time (even if it would be cool to be able to edit a menu while the conference is in progress).</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The following extensions were tested for successful entry with the expected options as well as not having any obvious memory leaks:
exten => 6060,1,NoOp
exten => 6060,n,Set(CONFBRIDGE(menu,*)=playback_and_continue(conf-usermenu))
exten => 6060,n,Set(CONFBRIDGE(menu,0)=leave_conference)
exten => 6060,n,Set(CONFBRIDGE(menu,*9)=increase_volume)
exten => 6060,n,Set(CONFBRIDGE(menu,*7)=decrease_volume)
exten => 6060,n,Answer()
exten => 6060,n,Confbridge(this_conference)
exten => 6061,1,Answer()
exten => 6061,n,Confbridge(this_conference)
exten => 6062,1,Answer()
exten => 6062,n,Confbridge(this_conference,,,sample_user_menu)
exten => 6063,1,NoOp
exten => 6063,n,Set(CONFBRIDGE(menu,template)=sample_user_menu)
exten => 6063,n,Set(CONFBRIDGE(menu,0)=leave_conference)
exten => 6063,n,Answer()
exten => 6063,n,Confbridge(this_conference)
With the following sample_user_menu configuration in confbridge.conf
[sample_user_menu]
type=menu
*=playback_and_continue(conf-usermenu)
*1=toggle_mute
1=toggle_mute
*4=decrease_listening_volume
4=decrease_listening_volume
*6=increase_listening_volume
6=increase_listening_volume
*7=decrease_talking_volume
7=decrease_talking_volume
*8=leave_conference
8=leave_conference
*9=increase_talking_volume
9=increase_talking_volume</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/apps/confbridge/include/confbridge.h <span style="color: grey">(402126)</span></li>
<li>/trunk/apps/confbridge/conf_config_parser.c <span style="color: grey">(402126)</span></li>
<li>/trunk/apps/app_confbridge.c <span style="color: grey">(402126)</span></li>
<li>/trunk/CHANGES <span style="color: grey">(402126)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2971/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>