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

[gareth]

> On April 13, 2015, 7:41 p.m., Corey Farrell wrote:
> > So I think this looks pretty good.  Next steps:
> > * We've migrated to git.  Take a look at [1] for information on how to use gerrit to post a git review.  Don't worry you won't be facing the full review again, we've already dealt with the code issues.
> > * We need to prepare a patch for starpy [2].  I don't have time to work on this right now.  Do you know python or know someone who does and has time/willingness to help?
> > * Verify the full testsuite passes against the latest Asterisk with this change.
> > * Once starpy supports the change we will coordinate with Digium to ensure starpy is updated before we commit this to trunk.
> > 
> > I know this seems like a lot, but we are changing the protocol, so we need to make sure we do not break the testsuite.
> > 
> > [1] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage
> > [2] https://github.com/asterisk/starpy/
> 
> Matt Jordan wrote:
>     I'd be willing to take on the starpy issues if/when this gets pulled over.

I wouldn't claim to know python, but I have sent a pull request on github that changes starpy to use the new format.

The testsuite only appears to use the command action in the apps/queues/set_penalty test and it doesn't even parse the output. The LUA AMI library in the testsuite checks for 'Response: Follows' so it shouldn't be affected by the new output format.


> On April 13, 2015, 7:41 p.m., Corey Farrell wrote:
> > /trunk/main/manager.c, line 4904
> > <https://reviewboard.asterisk.org/r/4391/diff/6/?file=73873#file73873line4904>
> >
> >     Nit pick: "Success" or "Error" is already provided through a standard field, so this could be static - "Message: Command Output Follows\r\n".  This would give a single value for Message that can be matched to determine that we successfully processed output, even if there are 0 lines or if the command provided the reason for failure in output.

I will change this when I post the review to gerrit.


- gareth


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


On April 13, 2015, 5 p.m., gareth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4391/
> -----------------------------------------------------------
> 
> (Updated April 13, 2015, 5 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 434448 
>   /trunk/main/cli.c 434448 
>   /trunk/include/asterisk/manager.h 434448 
>   /trunk/UPGRADE.txt 434448 
> 
> 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/20150414/5eb4827b/attachment-0001.html>