> On April 3, 2015, 11:37 p.m., Corey Farrell wrote: > > /trunk/main/manager.c, line 4873 > > <https://reviewboard.asterisk.org/r/4391/diff/3/?file=72904#file72904line4873> > > > > 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.
If the caller is executing any of the '<module> show ...' commands then they likely care about the output. In this case, I think it would be better to provide a more descriptive error message to the caller so they can detect if the command was executed. Yes, ast_cli_commmand_full should indicate whether the command failed, I will modify it so that an Error response can sent if the command fails. There don't appear to be any callers of that function who check the return code. - gareth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/#review15039 ----------------------------------------------------------- On March 25, 2015, 5:34 p.m., gareth wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4391/ > ----------------------------------------------------------- > > (Updated March 25, 2015, 5: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 > >
-- _____________________________________________________________________ -- 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
