[asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response

[Corey Farrell]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14781
-----------------------------------------------------------


I like this change mostly as is.  Since we are talking about changing the protocol I believe input from others is important.  We will need to work with starpy to ensure it can deal with the updated AMI protocol before we commit this change, and ensure it gets upgraded on Bamboo.  Remember the full testsuite needs to continue working in trunk.

On Matt's question about if we need to bump the version, I think we do, otherwise how would an AMI client know to parse the old output blob or the new Output: headers.


/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/4391/#comment25362>

    astman_start_ack(s, m) seems to be the standard way to begin success responses.  We would send Response: Success instead of Follows, but this is more accurate now that this is a normal response packet anyways.  astman_start_ack also doesn't send a Privilege: Command header, we could add that after (or not?).  The Privilege header is mostly just used for event packets, not action responses.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/4391/#comment25363>

    Is it safe to assume that CLI commands will never output "\r"?  If not we need to improve new-line detection to find and strip "\r".  I believe the regex we need is:
    (\n|\r\n|\r)
    
    This would ensure we don't get an any extra "\r" in output, and all text is put into headers.
    
    This idea is based on the belief that it's not valid for a header to have "\r" within its value, though I'm not positive on this.


- Corey Farrell


On March 19, 2015, 10:34 p.m., gareth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4391/
> -----------------------------------------------------------
> 
> (Updated March 19, 2015, 10:34 p.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 433198 
>   /trunk/include/asterisk/manager.h 433198 
>   /trunk/UPGRADE.txt 433198 
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150324/819e72c2/attachment.html>