<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 January 29th, 2015, 7:22 p.m. CST, <b>George Joseph</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 you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.</pre>
</blockquote>
<p>On January 29th, 2015, 10:29 p.m. CST, <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;">I ran the test apps/queue/set_penalty which makes use of ami.command and it failed.
However this appears to be a bug with starpy, it is treading the "\r\n" as the end of message. Changing it to output just "\n" results in a successful test.
Breaking an existing client library isn't ideal, but the correct delimiter for command output is "--END COMMAND--\r\n\r\n", not "\r\n\r\n".</pre>
</blockquote>
<p>On March 13th, 2015, 2:25 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;">If we do consider modifying the AMI protocol to support text blobs like this, I'd prefer we think about how that should be implemented generically, rather than looking at this one action. So maybe adding 'Text-Terminator: --END COMMAND--' or 'Content-Length: XX' to the headers would allow the next packet to be read as a text block. This way the AMI client libraries could have generic support for this type of response. On the other hand AMI is not HTTP. This is a perfect example of something ARI is better suited for. For that reason I'm not actually in favor of having AMI support text blobs.
I'd rather see the command output converted to headers:
Response: Success
ActionID: <actionid>
Output: line 1 of cli output
Output: more Output headers per line of cli output
This would of course break existing clients, but would get rid of the exception to the AMI protocol.</pre>
</blockquote>
<p>On March 15th, 2015, 8:13 p.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;">Switching to a header-based output would be much better, the current output format seems to lend itself to being mis-parsed.
Adding a header to the request would allow users to choose the new format, eg:
Action: Command
Command: ...
OutputHeader: true</pre>
</blockquote>
<p>On March 15th, 2015, 9:19 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'm not sure about giving an option. I think if we're bumping the AMI version because of an incompatible change, we should just get rid of what was broken. That's just my opinion though, it's worth hearing opinions of others.</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;">So, I agree with Corey. I think if we're going to fix this and bump the version number, then let's just bite the bullet and do it.
My suggestion is to do the following:
1) Make sure we are happy with this patch and that it is ready to go.
2) When that occurs, we should make a note on the wiki page here:
https://wiki.asterisk.org/wiki/display/AST/Asterisk+API+Improvements
3) Sometime prior to cutting a new major branch, we should get together on the -dev list and discuss whether or not it is worth bumping the major version. Given the other things on the list, I think the answer will be yes - and if you think it is worth having the discussion now, then we certainly can start it.</pre>
<br />
<p>- Matt</p>
<br />
<p>On January 29th, 2015, 6:25 p.m. CST, 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 Jan. 29, 2015, 6:25 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-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">(431113)</span></li>
<li>/trunk/include/asterisk/manager.h <span style="color: grey">(431113)</span></li>
<li>/trunk/CHANGES <span style="color: grey">(431113)</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>