<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/4391/">https://reviewboard.asterisk.org/r/4391/</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 3rd, 2015, 5:37 a.m. CDT, <b>Corey Farrell</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/4391/diff/3/?file=72904#file72904line4873" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/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">4867</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>astman_send_error(s, m, "Command response construction error");</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;">If we successfully ran the command, it seems unsafe to claim failure. We have to assume the the caller doesn't actually care about any of the CLI output, they only care about having the command perform an action. So I think we need to respond with success if the command ran. I'm leaning towards thinking that this error should be provided through a single "Output: Command response construction error\r\n", so move astman_start_ack to just below ast_cli_command.
On a related issue, there are a couple errors that can occur in ast_cli_command_full which print error messages and return success. I don't know if it's safe to modify ast_cli_command_full to return errors for more situations, it might be worth looking at to allow us to trust the return value of ast_cli_command_full. CLI commands themselves can return an error, but this error is not returned by ast_cli_command_full. It would be nice if this action could use the return value from ast_cli_command_full to determine if it should respond success or failure.</pre>
</blockquote>
<p>On April 9th, 2015, 12:04 a.m. CDT, <b>gareth</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;">If the caller is executing any of the '<module> show ...' commands then they likely care about the output. In this case, I think it would be better to provide a more descriptive error message to the caller so they can detect if the command was executed.
Yes, ast_cli_commmand_full should indicate whether the command failed, I will modify it so that an Error response can sent if the command fails. There don't appear to be any callers of that function who check the return code.</pre>
</blockquote>
<p>On April 9th, 2015, 3:37 p.m. CDT, <b>Corey Farrell</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 do not feel this issue is resolved. If a command has side-effects, we should respond with Success/Failure based on if the command ran - regardless of our (in)ability to process output. Since there is no way to determine if a CLI command is meant to retrieve information or manipulate it, we have to assume that it is manipulating something. So I agree that descriptive error messages are useful, we need to avoid lying to the caller by claiming that the command failed to run if it did run.
For times where people are just retrieving information, couldn't we respond with success, put the error in 'Message:', and provide no 'Output:' headers? This way if you care about the output, you can detect the lack of output.</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;">This is a tricky one, but I think I agree with Corey on this.
We will already send an Error with a Message value if the command was executed but returned a failure response code. The fact that we executed the command successfully but had an internal error that precluded showing the response feels like it should be different from conditions where we either never ran the command at all.
How about changing the astman_send_error to the Message: that is sent with a status of Error? In those cases, there wouldn't be any response that follows - since we can't send it - but there is still an overall indication that the command succeeded (sorta).</pre>
<br />
<p>- Matt</p>
<br />
<p>On April 9th, 2015, 12:05 a.m. CDT, gareth 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.</div>
<div>By gareth.</div>
<p style="color: grey;"><i>Updated April 9, 2015, 12:05 a.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-24730">ASTERISK-24730</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 patch adds a blank line between the headers and the output in the Command action response. The reason for this change is to make it easier to determine where the headers end and the output from the command starts.
Currently, to parse a response to a Command action, one has to:
1. Read one line, if line is "Response: Error", parse the remaining as a standard AMI response and stop.
2. Read one more line - or two if you used an ActionID - those lines are the headers.
3. Then read everything up to "--END COMMAND--\r\n\r\n".
That could be changed to:
1. Read standard AMI response.
2. If "Response: Follows", then read everything up to "--END COMMAND--\r\n\r\n".
The AMI version has been increased to 2.8.0 as this is a protocol change and so that clients detect the new behavior.
Adding a blank line should not cause older parsers to fail as they have to read everything up to "--END COMMAND--\r\n\r\n" anyway.
Adding a blank line will also not cause the AMI to HTML/XML encoder to fail to separate the headers from the output as it checks for the presence of a ":" character, which a blank line does not contain.</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;">Connected to manager, issued 'core show uptime' command and verified that there was a blank line between the headers and output.</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/main/manager.c <span style="color: grey">(434448)</span></li>
<li>/trunk/main/cli.c <span style="color: grey">(434448)</span></li>
<li>/trunk/include/asterisk/manager.h <span style="color: grey">(434448)</span></li>
<li>/trunk/UPGRADE.txt <span style="color: grey">(434448)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4391/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>