[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/#review15039
-----------------------------------------------------------



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

    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.


- Corey Farrell


On March 25, 2015, 12:34 a.m., gareth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4391/
> -----------------------------------------------------------
> 
> (Updated March 25, 2015, 12:34 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 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/20150403/1ca86345/attachment.html>