<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/2922/">https://reviewboard.asterisk.org/r/2922/</a>
</td>
</tr>
</table>
<br />
<div>
<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/2922/diff/1/?file=47149#file47149line189" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/res/res_stasis_recording.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">enum ast_record_if_exists stasis_app_recording_if_exists_parse(</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">189</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">RAII_VAR</span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_channel_snapshot</span> <span class="o">*</span><span class="p">,</span> <span class="n">snapshot</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">,</span> <span class="n">ao2_cleanup</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">189</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">RAII_VAR</span><span class="p">(</span><span class="k">struct</span> <span class="n">ast_channel_snapshot</span> <span class="o">*</span><span class="p">,</span> <span class="n">snapshot</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">,</span> <span class="n">ao2_cleanup</span><span class="p">);</span><span class="hl"> </span><span class="cm"><span class="hl">/* </span></span><span class="cs"><span class="hl">XXX</span></span><span class="cm"><span class="hl"> Why does this exist? It seems unused */</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This comment doesn't really pertain to the rest of the review. If the variable is unused, then it can be removed in a separate commit.</pre>
</div>
<br />
<div>
<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/2922/diff/1/?file=47149#file47149line282" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/res/res_stasis_recording.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">static void *record_file(struct stasis_app_control *control,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">282</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">ast_debug</span><span class="p">(</span><span class="mi">3</span><span class="p">,</span> <span class="s">"%s: Recording failed for some internal reason.</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">ast_channel_uniqueid</span><span class="p">(</span><span class="n">chan</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think that a failure should result in a more prominent log level than a debug message. Probably a warning would suit this better.</pre>
</div>
<br />
<div>
<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/2922/diff/1/?file=47149#file47149line375" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/res/res_stasis_recording.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">struct stasis_app_recording *stasis_app_control_record(</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">363</font></th>
<td bgcolor="#fdfebc" 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">errno</span> <span class="o">=</span> <span class="n">E<span class="hl">EXIST</span></span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">375</font></th>
<td bgcolor="#fdfebc" 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">errno</span> <span class="o">=</span> <span class="n">E<span class="hl">AGAIN</span></span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hm, I'm not too keen on the use of EAGAIN for this. EAGAIN is typically set if a non-blocking operation would block if attempted. This doesn't really fit here. I think there doesn't need to be a disambiguation between whether the recording exists on the filesystem or if the recording is in progress. In the eyes of most users, the problem is the same, which is that there is a resource conflict. I think this change can be reverted to use EEXIST.</pre>
</div>
<br />
<div>
<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/2922/diff/1/?file=47150#file47150line468" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/rest-api/api-docs/bridges.json</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">468</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nt">"reason"</span><span class="p">:</span> <span class="s2">"Bridge is not in a Stasis application; A recording with the same name is currently in progress; A file with the same name the recording would use already exists and ifExists is set to fail"</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">My opinion:
s/and ifExists is set to fail/overwriting is not permitted/
similar to the wording you have in res/ari/resource_channels.c</pre>
</div>
<br />
<div>
<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/2922/diff/1/?file=47151#file47151line723" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/rest-api/api-docs/channels.json</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">723</font></th>
<td bgcolor="#fdfebc" 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="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="nt">"reason"</span><span class="p">:</span> <span class="s2">"Channel is not in a Stasis application; the channel is currently bridged with other channels; A recording with the same name is currently in progress<span class="hl">."</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">723</font></th>
<td bgcolor="#fdfebc" 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="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="nt">"reason"</span><span class="p">:</span> <span class="s2">"Channel is not in a Stasis application; the channel is currently bridged with other channels; A recording with the same name is currently in progress<span class="hl">; A file with the same name the recording would use already exists and ifExists is set to fail"</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ditto for this.</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On October 16th, 2013, 8:48 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.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, David Lee and kmoore.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Oct. 16, 2013, 8:48 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-22623">ASTERISK-22623</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;">This affects both channel and bridge recording. Basically it just adds a simple check to make sure that the requested file doesn't already exist on the file system when we want to record. If it does, we set EEXIST on errno and return error. I went ahead and switched the errno value for when another live recording exists that uses the same base name to EAGAIN... not sure how appropriate that is.
I also added event models for Recording events, but nothing issues these events yet. They'll be useful later.
I've written a test for this, which is in review here: https://reviewboard.asterisk.org/r/2921/</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;">I tested it manually with Swagger UI and attempted to record when a file existed where I wanted one and received the expected 409. I also checked to make sure I would still succeed if ifExists was set to overwrite. I also checked for success when the file just didn't exist in the first place. I think that covers all my bases. All of these scenarios are also covered by the automated test in review 2921</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>/branches/12/res/ari/ari_model_validators.h <span style="color: grey">(400905)</span></li>
<li>/branches/12/res/ari/ari_model_validators.c <span style="color: grey">(400905)</span></li>
<li>/branches/12/res/ari/resource_bridges.c <span style="color: grey">(400905)</span></li>
<li>/branches/12/res/ari/resource_channels.c <span style="color: grey">(400905)</span></li>
<li>/branches/12/res/res_ari_bridges.c <span style="color: grey">(400905)</span></li>
<li>/branches/12/res/res_ari_channels.c <span style="color: grey">(400905)</span></li>
<li>/branches/12/res/res_stasis_recording.c <span style="color: grey">(400905)</span></li>
<li>/branches/12/rest-api/api-docs/bridges.json <span style="color: grey">(400905)</span></li>
<li>/branches/12/rest-api/api-docs/channels.json <span style="color: grey">(400905)</span></li>
<li>/branches/12/rest-api/api-docs/events.json <span style="color: grey">(400905)</span></li>
<li>/branches/12/rest-api/api-docs/recordings.json <span style="color: grey">(400905)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2922/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>