<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/1863/">https://reviewboard.asterisk.org/r/1863/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 13th, 2012, 2:42 p.m., <b>rmudgett</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/1863/diff/3/?file=27253#file27253line3624" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/manager.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; ">static int action_command(struct mansession *s, const struct message *m)</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">3624</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">((</span><span class="n">l</span> <span class="o">=</span> <span class="n">lseek</span><span class="p">(</span><span class="n">fd</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">SEEK_END</span><span class="p">))</span> <span class="o"><</span> <span class="mi">0</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<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">3625</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_WARNING</span><span class="p">,</span> <span class="s">"Failed to determine number of characters for command: %s</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">strerror</span><span class="p">(</span><span class="n">errno</span><span class="p">));</span></pre></td>
</tr>
<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">3626</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">astman_send_error</span><span class="p">(</span><span class="n">s</span><span class="p">,</span> <span class="n">m</span><span class="p">,</span> <span class="s">"Command response construction error"</span><span class="p">);</span></pre></td>
</tr>
<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">3627</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">goto</span> <span class="n">action_command_cleanup</span><span class="p">;</span></pre></td>
</tr>
<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">3628</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3620</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3629</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3621</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* This has a potential to overflow the stack. Hence, use the heap. */</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">3630</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/* This has a potential to overflow the stack. Hence, use the heap. */</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3622</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">buf</span> <span class="o">=</span> <span class="n">ast_calloc</span><span class="p">(</span><span class="mi">1</span><span class="p">,</span> <span class="n">l</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">3631</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">buf</span> <span class="o">=</span> <span class="n">ast_calloc</span><span class="p">(</span><span class="mi">1</span><span class="p">,</span> <span class="n">l</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">3623</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">final_buf</span> <span class="o">=</span> <span class="n">ast_calloc</span><span class="p">(</span><span class="mi">1</span><span class="p">,</span> <span class="n">l</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">3632</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">final_buf</span> <span class="o">=</span> <span class="n">ast_calloc</span><span class="p">(</span><span class="mi">1</span><span class="p">,</span> <span class="n">l</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">3624</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">buf</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">3633</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">buf</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3625</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">lseek</span><span class="p">(</span><span class="n">fd</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">SEEK_SET</span><span class="p">)<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">3634</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"> </span><span class="k"><span class="hl">if</span></span><span class="hl"> </span><span class="p"><span class="hl">(</span></span><span class="n">lseek</span><span class="p">(</span><span class="n">fd</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">SEEK_SET</span><span class="p">)</span><span class="hl"> </span><span class="o"><span class="hl"><</span></span><span class="hl"> </span><span class="mi"><span class="hl">0</span></span><span class="p"><span class="hl">)</span></span><span class="hl"> </span><span class="p"><span class="hl">{</span></span></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">3635</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_WARNING</span><span class="p">,</span> <span class="s">"Failed to set position on temporary file for command: %s</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">strerror</span><span class="p">(</span><span class="n">errno</span><span class="p">));</span></pre></td>
</tr>
<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">3636</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">astman_send_error</span><span class="p">(</span><span class="n">s</span><span class="p">,</span> <span class="n">m</span><span class="p">,</span> <span class="s">"Command response construction error"</span><span class="p">);</span></pre></td>
</tr>
<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">3637</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_free</span><span class="p">(</span><span class="n">buf</span><span class="p">);</span></pre></td>
</tr>
<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">3638</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">goto</span> <span class="n">action_command_cleanup</span><span class="p">;</span></pre></td>
</tr>
<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">3639</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3626</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">read</span><span class="p">(</span><span class="n">fd</span><span class="p">,</span> <span class="n">buf</span><span class="p">,</span> <span class="n">l</span><span class="p">)</span> <span class="o"><</span> <span class="mi">0</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">3640</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">read</span><span class="p">(</span><span class="n">fd</span><span class="p">,</span> <span class="n">buf</span><span class="p">,</span> <span class="n">l</span><span class="p">)</span> <span class="o"><</span> <span class="mi">0</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">3627</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_WARNING</span><span class="p">,</span> <span class="s">"read() failed: %s</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">strerror</span><span class="p">(</span><span class="n">errno</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">3641</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_WARNING</span><span class="p">,</span> <span class="s">"read() failed: %s</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">strerror</span><span class="p">(</span><span class="n">errno</span><span class="p">));</span></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">3642</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">astman_send_error</span><span class="p">(</span><span class="n">s</span><span class="p">,</span> <span class="n">m</span><span class="p">,</span> <span class="s">"Command response construction error"</span><span class="p">);</span></pre></td>
</tr>
<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">3643</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_free</span><span class="p">(</span><span class="n">buf</span><span class="p">);</span></pre></td>
</tr>
<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">3644</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">goto</span> <span class="n">action_command_cleanup</span><span class="p">;</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3628</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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">3645</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <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;">You cannot use astman_send_error here since you have a partially created command response already. You need to use astman_append instead to output your error message as part of the command response.</pre>
</blockquote>
<p>On April 13th, 2012, 3:36 p.m., <b>Matt Jordan</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;">I'm not sure about that, although I agree that astman_send_error without first terminating the in flight event isn't correct either.
As I see it, we have three options:
1. When an error occurs during response construction, we immediately terminate the current response, and send an error message. This would look like:
Response: Follows
Privilege: Command
ActionID: 1234
blah blah blah yackity I'm a CLI output
and now I've failed
--END COMMAND--
Response: Failure
ActionID: 1234
Message: Command response construction error
2. When an error occurs during response construction, we instead embed the failure as a sort of warning in the response. This would look like:
Response: Follows
Privilege: Command
ActionID: 1234
blah blah blah yackity I'm a CLI output
and now I've failed
--END COMMAND--
Message: Command response construction error
3. Or, we attempt to buffer the whole CLI response in memory before sending it. This would allows us to either send the whole thing successfully, or an error response. I'm concerned about this approach only because the CLI output could be rather largish (memory allocations comes to mind), and trying to buffer it all in memory may be a bad idea.
My preference would be to do #1, since it doesn't mess around with any current response parsing, and both messages are in the format currently expected. I'm not sure if anyone expects to get two Response events however.
</pre>
</blockquote>
<p>On April 13th, 2012, 4:29 p.m., <b>Matt Jordan</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;">After some discussion, there isn't a very good approach to this problem that doesn't change existing behavior. Instead of trying to send an error message back to the connected AMI session, the least intrusive approach will probably be to just end the message with an "--END COMMAND--" and log an error in Asterisk.</pre>
</blockquote>
<p>On April 15th, 2012, 3:28 p.m., <b>Tilghman Lesher</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;">We already buffer the entire CLI response. That's what this file is: the contents of the entire output of the CLI command. We do this because CLI commands have the potential to take up quite a bit of time (for example, dealing with channel locks when executing 'core show channels'), and we don't want to lock out all asynchronous events on a particular AMI connection (if we didn't lock, we could get other events in the middle of our CLI output).
The rest of the code is dealing with the possible error of not being able to set the file position back to 0 on a file created with mkstemp(), or reading the file back out of that temporary file, which is likely all cached in kernel memory anyway. As far as error checking, for lseek(), there are four on the Linux manpage: EBADF is the only one that seems possible, since we don't check the FD returned from mkstemp for validity (in which case, we should fail sooner). All of the others deal with invalid inputs, which are not possible with these particular constant inputs. For the read(), we should probably be checking for EINTR and restarting, which is what the current code effectively does. EIO is the only other possible error, and given that the entire file is cached in memory, it also seems unlikely.
I understand that you're eliminating warnings from a static checker, and that's fine. But the only possible error from lseek() should be caught on the mkstemp(), before ast_cli_command() is called. Add to this the case of EIO on the read(), and I don't think we need to take error handling any further.</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;">Going off your paragraphs:
1. I know that the temporary file is buffering the return of the CLI command. What I was referring to in my "option 3" was to modify things such that an in memory buffer would instead be filled directly by the CLI command. I don't think this is necessarily a good idea - it would, if nothing else, entail large architectural changes that are certainly not worth the risk. I was trying to think of ways that we could make it such that the operations between execution of the CLI command and sending the resulting data won't fail - but I'm not really sure that even working the temporary file out of the way would have that benefit.
2. I agree - for lseek EBADF seems to be the only error condition that could occur, which is highly unlikely now that we're also checking for failure of mkstemp. I'd prefer to keep a check in here anyway, simply to quiet the static analysis tools (and it really doesn't hurt anything to have it there). With respect to the read opreation, if I'm reading your comment correctly, then I disagree that the current code handles EINTR with a restart - based on my reading, if we have any failure, we would append a buffer of NULL characters to the manager response and send the --END COMMAND--. I'd prefer not to add any re-try logic in here as well, if for no other reason then doing so in a release branch doesn't seem warranted here. I'd prefer to let the operation just fail, and have the manager user retry the command if the output of the CLI command happens to be empty.
In general, I prefer not to try and predict what goofiness can happen and instead handle the possible conditions that arise. Sometimes that does mean that there are error checks that stand next to no realistic chance of being executed (most of the ones in this review fall in that category), but unless there's a huge performance penalty to pay for it, I don't see any reason not to have them. Since we're already potentially reading in a very large file in this operation, performance penalties seem a very unlikely reason not to check the returns of lseek/read.</pre>
<br />
<p>- Matt</p>
<br />
<p>On April 13th, 2012, 4:46 p.m., Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/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.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated April 13, 2012, 4:46 p.m.</i></p>
<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;">Through a static analysis tool, a large number of errors were found that dealt with using potential negative return values in harmful ways. The following is a summary of the changes for the various affected files:
* app_voicemail
- a negative result from lseek is later passed into mmap as the size of the memory map
- a negative result from read closes the various file descriptors and unlinks the output file, but the next check against the return value only checks to see if its non-zero
* chan_agent - ast_channel_unlock returns a negative value if the item passed to it is NULL or an invalid ao2 object. strerror cannot accept a negative value.
* chan_dahdi - if dahdi_get_index returns a negative value, we index directly into an array
* format_* - various errors not checking return values of ftello and fseek, and passing the results into functions that expect non-negative values
* func_env - various places where the result of ftello was directly used as input to functions that expect non-negative values
* asterisk
- the request to read returns a negative value. This causes us to enter into the retry logic. If we attempt to reconnect and succeed, we would normally proceed in the for(;;) loop and attempt to index into buf using the negative return value from read. Instead, if we do reconnect, we immediately return to the beginning of the for(;;) loop and attempt a new read.
- similar situation - if a read fails, don't attempt to index into a buffer using the return value
* frame - if we can't determine a preferred codec using the provided values, don't attempt to use an index value that never got set
* manager
- various failures of mkstemp, lseek were not checked for and could be provided to methods that don't handle negative numbers
- passing a negative result from lseek into mmap as the size of the memory map
* translate - powerof can return a negative result if no bits are set, which would then be used as an index into an array
* res_agi - if read returns an error, we treated it as if bytes were read from the pipe
* res_musiconhold - if we fail to spawn spawn_mp3 returns a negative number, we wait a bit and attempt again later. However, the return 'file descriptor' srcfd is later passed into read.
* res_rtp_asterisk - ast_rtp_codecs_payload_code can return a negative value if no codecs are found that match between instance1 and the specified payload. If that's the case, the bridge should be broken, as there are no compatible formats between the two different endpoints (and we shouldn't index into an array using the return value)</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/1.8/apps/app_voicemail.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/channels/chan_agent.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/channels/chan_dahdi.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_g719.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_g723.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_g729.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_gsm.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_h263.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_h264.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_ilbc.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_pcm.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_siren14.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_siren7.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_sln.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_sln16.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_vox.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_wav.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/formats/format_wav_gsm.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/funcs/func_env.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/main/asterisk.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/main/frame.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/main/manager.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/main/translate.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/res/res_agi.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/res/res_musiconhold.c <span style="color: grey">(362134)</span></li>
<li>/branches/1.8/res/res_rtp_asterisk.c <span style="color: grey">(362134)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1863/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>