> 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 > >
-- _____________________________________________________________________ -- 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
