> On Jan. 30, 2015, 1:22 a.m., George Joseph wrote: > > If you run the Testsuite, you'll get a good indication of whether this is > > truly backwards compatible. > > gareth wrote: > 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". > > Corey Farrell wrote: > 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. > > gareth wrote: > 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 > > Corey Farrell wrote: > 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. > > Matt Jordan wrote: > 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.
Okay, the old output format will be removed. While there, I might as well fix another problem with action_command where it does not send back an error response if malloc, lseek or read fail. - gareth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/#review14384 ----------------------------------------------------------- On Jan. 30, 2015, 12:25 a.m., gareth wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4391/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2015, 12:25 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24730 > https://issues.asterisk.org/jira/browse/ASTERISK-24730 > > > Repository: Asterisk > > > Description > ------- > > 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. > > > Diffs > ----- > > /trunk/main/manager.c 431113 > /trunk/include/asterisk/manager.h 431113 > /trunk/CHANGES 431113 > > Diff: https://reviewboard.asterisk.org/r/4391/diff/ > > > Testing > ------- > > Connected to manager, issued 'core show uptime' command and verified that > there was a blank line between the headers and output. > > > Thanks, > > gareth > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
