Discussion:
[Code Review] 4391: Add blank line between headers and output for Command action response
(too old to reply)
gareth
2015-01-30 00:25:56 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------

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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113

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
George Joseph
2015-01-30 01:22:53 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14384
-----------------------------------------------------------


If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.

- George Joseph
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated Jan. 29, 2015, 5:25 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.
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".
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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113
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
gareth
2015-01-30 04:29:53 UTC
Permalink
Post by gareth
Post by George Joseph
If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.
I ran the test apps/queue/set_penalty which makes use of ami.command and it failed.

However this appears to be a bug with starpy, it is treading the "\r\n" as the end of message. Changing it to output just "\n" results in a successful test.

Breaking an existing client library isn't ideal, but the correct delimiter for command output is "--END COMMAND--\r\n\r\n", not "\r\n\r\n".


- gareth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14384
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated Jan. 30, 2015, 12:25 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.
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".
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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113
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
Corey Farrell
2015-03-13 19:25:51 UTC
Permalink
Post by gareth
Post by George Joseph
If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.
I ran the test apps/queue/set_penalty which makes use of ami.command and it failed.
However this appears to be a bug with starpy, it is treading the "\r\n" as the end of message. Changing it to output just "\n" results in a successful test.
Breaking an existing client library isn't ideal, but the correct delimiter for command output is "--END COMMAND--\r\n\r\n", not "\r\n\r\n".
If we do consider modifying the AMI protocol to support text blobs like this, I'd prefer we think about how that should be implemented generically, rather than looking at this one action. So maybe adding 'Text-Terminator: --END COMMAND--' or 'Content-Length: XX' to the headers would allow the next packet to be read as a text block. This way the AMI client libraries could have generic support for this type of response. On the other hand AMI is not HTTP. This is a perfect example of something ARI is better suited for. For that reason I'm not actually in favor of having AMI support text blobs.

I'd rather see the command output converted to headers:
Response: Success
ActionID: <actionid>
Output: line 1 of cli output
Output: more Output headers per line of cli output

This would of course break existing clients, but would get rid of the exception to the AMI protocol.


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14384
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated Jan. 29, 2015, 7:25 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.
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".
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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113
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
gareth
2015-03-16 01:13:34 UTC
Permalink
Post by gareth
Post by George Joseph
If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.
I ran the test apps/queue/set_penalty which makes use of ami.command and it failed.
However this appears to be a bug with starpy, it is treading the "\r\n" as the end of message. Changing it to output just "\n" results in a successful test.
Breaking an existing client library isn't ideal, but the correct delimiter for command output is "--END COMMAND--\r\n\r\n", not "\r\n\r\n".
If we do consider modifying the AMI protocol to support text blobs like this, I'd prefer we think about how that should be implemented generically, rather than looking at this one action. So maybe adding 'Text-Terminator: --END COMMAND--' or 'Content-Length: XX' to the headers would allow the next packet to be read as a text block. This way the AMI client libraries could have generic support for this type of response. On the other hand AMI is not HTTP. This is a perfect example of something ARI is better suited for. For that reason I'm not actually in favor of having AMI support text blobs.
Response: Success
ActionID: <actionid>
Output: line 1 of cli output
Output: more Output headers per line of cli output
This would of course break existing clients, but would get rid of the exception to the AMI protocol.
Switching to a header-based output would be much better, the current output format seems to lend itself to being mis-parsed.

Adding a header to the request would allow users to choose the new format, eg:

Action: Command
Command: ...
OutputHeader: true


- gareth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14384
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated Jan. 30, 2015, 12:25 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.
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".
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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113
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
Corey Farrell
2015-03-16 02:19:59 UTC
Permalink
Post by gareth
Post by George Joseph
If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.
I ran the test apps/queue/set_penalty which makes use of ami.command and it failed.
However this appears to be a bug with starpy, it is treading the "\r\n" as the end of message. Changing it to output just "\n" results in a successful test.
Breaking an existing client library isn't ideal, but the correct delimiter for command output is "--END COMMAND--\r\n\r\n", not "\r\n\r\n".
If we do consider modifying the AMI protocol to support text blobs like this, I'd prefer we think about how that should be implemented generically, rather than looking at this one action. So maybe adding 'Text-Terminator: --END COMMAND--' or 'Content-Length: XX' to the headers would allow the next packet to be read as a text block. This way the AMI client libraries could have generic support for this type of response. On the other hand AMI is not HTTP. This is a perfect example of something ARI is better suited for. For that reason I'm not actually in favor of having AMI support text blobs.
Response: Success
ActionID: <actionid>
Output: line 1 of cli output
Output: more Output headers per line of cli output
This would of course break existing clients, but would get rid of the exception to the AMI protocol.
Switching to a header-based output would be much better, the current output format seems to lend itself to being mis-parsed.
Action: Command
Command: ...
OutputHeader: true
I'm not sure about giving an option. I think if we're bumping the AMI version because of an incompatible change, we should just get rid of what was broken. That's just my opinion though, it's worth hearing opinions of others.


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14384
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated Jan. 29, 2015, 7:25 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.
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".
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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113
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
Matt Jordan
2015-03-19 20:50:48 UTC
Permalink
Post by gareth
Post by George Joseph
If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.
I ran the test apps/queue/set_penalty which makes use of ami.command and it failed.
However this appears to be a bug with starpy, it is treading the "\r\n" as the end of message. Changing it to output just "\n" results in a successful test.
Breaking an existing client library isn't ideal, but the correct delimiter for command output is "--END COMMAND--\r\n\r\n", not "\r\n\r\n".
If we do consider modifying the AMI protocol to support text blobs like this, I'd prefer we think about how that should be implemented generically, rather than looking at this one action. So maybe adding 'Text-Terminator: --END COMMAND--' or 'Content-Length: XX' to the headers would allow the next packet to be read as a text block. This way the AMI client libraries could have generic support for this type of response. On the other hand AMI is not HTTP. This is a perfect example of something ARI is better suited for. For that reason I'm not actually in favor of having AMI support text blobs.
Response: Success
ActionID: <actionid>
Output: line 1 of cli output
Output: more Output headers per line of cli output
This would of course break existing clients, but would get rid of the exception to the AMI protocol.
Switching to a header-based output would be much better, the current output format seems to lend itself to being mis-parsed.
Action: Command
Command: ...
OutputHeader: true
I'm not sure about giving an option. I think if we're bumping the AMI version because of an incompatible change, we should just get rid of what was broken. That's just my opinion though, it's worth hearing opinions of others.
So, I agree with Corey. I think if we're going to fix this and bump the version number, then let's just bite the bullet and do it.

My suggestion is to do the following:
1) Make sure we are happy with this patch and that it is ready to go.
2) When that occurs, we should make a note on the wiki page here:
https://wiki.asterisk.org/wiki/display/AST/Asterisk+API+Improvements
3) Sometime prior to cutting a new major branch, we should get together on the -dev list and discuss whether or not it is worth bumping the major version. Given the other things on the list, I think the answer will be yes - and if you think it is worth having the discussion now, then we certainly can start it.


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14384
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated Jan. 29, 2015, 6:25 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.
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".
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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113
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
gareth
2015-03-20 02:33:23 UTC
Permalink
Post by gareth
Post by George Joseph
If you run the Testsuite, you'll get a good indication of whether this is truly backwards compatible.
I ran the test apps/queue/set_penalty which makes use of ami.command and it failed.
However this appears to be a bug with starpy, it is treading the "\r\n" as the end of message. Changing it to output just "\n" results in a successful test.
Breaking an existing client library isn't ideal, but the correct delimiter for command output is "--END COMMAND--\r\n\r\n", not "\r\n\r\n".
If we do consider modifying the AMI protocol to support text blobs like this, I'd prefer we think about how that should be implemented generically, rather than looking at this one action. So maybe adding 'Text-Terminator: --END COMMAND--' or 'Content-Length: XX' to the headers would allow the next packet to be read as a text block. This way the AMI client libraries could have generic support for this type of response. On the other hand AMI is not HTTP. This is a perfect example of something ARI is better suited for. For that reason I'm not actually in favor of having AMI support text blobs.
Response: Success
ActionID: <actionid>
Output: line 1 of cli output
Output: more Output headers per line of cli output
This would of course break existing clients, but would get rid of the exception to the AMI protocol.
Switching to a header-based output would be much better, the current output format seems to lend itself to being mis-parsed.
Action: Command
Command: ...
OutputHeader: true
I'm not sure about giving an option. I think if we're bumping the AMI version because of an incompatible change, we should just get rid of what was broken. That's just my opinion though, it's worth hearing opinions of others.
So, I agree with Corey. I think if we're going to fix this and bump the version number, then let's just bite the bullet and do it.
1) Make sure we are happy with this patch and that it is ready to go.
https://wiki.asterisk.org/wiki/display/AST/Asterisk+API+Improvements
3) Sometime prior to cutting a new major branch, we should get together on the -dev list and discuss whether or not it is worth bumping the major version. Given the other things on the list, I think the answer will be yes - and if you think it is worth having the discussion now, then we certainly can start it.
Okay, the old output format will be removed.

While there, I might as well fix another problem with action_command where it does not send back an error response if malloc, lseek or read fail.


- gareth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14384
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated Jan. 30, 2015, 12:25 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.
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".
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 431113
/trunk/include/asterisk/manager.h 431113
/trunk/CHANGES 431113
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
gareth
2015-03-20 02:34:58 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------

(Updated March 20, 2015, 2:34 a.m.)


Review request for Asterisk Developers.


Changes
-------

New Patch Behaviour:

Output from the CLI command is now sent to the client as a series of "Output:" headers.

malloc, lseek and read errors will now cause an error response to be sent to the client rather than sending no output.


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 (updated)
-----

/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
Corey Farrell
2015-03-24 20:24:02 UTC
Permalink
-----------------------------------------------------------
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
Post by gareth
-----------------------------------------------------------
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.
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".
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
gareth
2015-03-25 04:33:36 UTC
Permalink
Post by gareth
/trunk/main/manager.c, lines 4908-4911
<https://reviewboard.asterisk.org/r/4391/diff/2/?file=72690#file72690line4908>
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.
Agreed.
Post by gareth
/trunk/main/manager.c, lines 4914-4916
<https://reviewboard.asterisk.org/r/4391/diff/2/?file=72690#file72690line4914>
(\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.
If the \r character occurs in the middle of the line, eg: "Hello\rWorld\n", splitting that into two output headers does not seem like the expected behaviour.

And by splitting on more than one character you could end up with a difference when reassembling the output: join('\n', output1, output2, ...) != output;


- gareth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review14781
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated March 20, 2015, 3: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.
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".
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
gareth
2015-03-25 04:34:55 UTC
Permalink
-----------------------------------------------------------
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.


Changes
-------

Switch to using astman_start_ack for the response.


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 (updated)
-----

/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
Corey Farrell
2015-04-03 10:37:45 UTC
Permalink
-----------------------------------------------------------
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
Post by gareth
-----------------------------------------------------------
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.
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".
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
gareth
2015-04-09 05:04:36 UTC
Permalink
Post by gareth
/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
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
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.
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".
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
Corey Farrell
2015-04-09 20:38:00 UTC
Permalink
Post by gareth
/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.
I do not feel this issue is resolved. If a command has side-effects, we should respond with Success/Failure based on if the command ran - regardless of our (in)ability to process output. Since there is no way to determine if a CLI command is meant to retrieve information or manipulate it, we have to assume that it is manipulating something. So I agree that descriptive error messages are useful, we need to avoid lying to the caller by claiming that the command failed to run if it did run.

For times where people are just retrieving information, couldn't we respond with success, put the error in 'Message:', and provide no 'Output:' headers? This way if you care about the output, you can detect the lack of output.


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15039
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 9, 2015, 1:05 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.
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".
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
Matt Jordan
2015-04-09 20:50:17 UTC
Permalink
Post by gareth
/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.
I do not feel this issue is resolved. If a command has side-effects, we should respond with Success/Failure based on if the command ran - regardless of our (in)ability to process output. Since there is no way to determine if a CLI command is meant to retrieve information or manipulate it, we have to assume that it is manipulating something. So I agree that descriptive error messages are useful, we need to avoid lying to the caller by claiming that the command failed to run if it did run.
For times where people are just retrieving information, couldn't we respond with success, put the error in 'Message:', and provide no 'Output:' headers? This way if you care about the output, you can detect the lack of output.
This is a tricky one, but I think I agree with Corey on this.

We will already send an Error with a Message value if the command was executed but returned a failure response code. The fact that we executed the command successfully but had an internal error that precluded showing the response feels like it should be different from conditions where we either never ran the command at all.

How about changing the astman_send_error to the Message: that is sent with a status of Error? In those cases, there wouldn't be any response that follows - since we can't send it - but there is still an overall indication that the command succeeded (sorta).


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15039
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 9, 2015, 12:05 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.
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".
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
gareth
2015-04-10 03:51:56 UTC
Permalink
Post by gareth
/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.
I do not feel this issue is resolved. If a command has side-effects, we should respond with Success/Failure based on if the command ran - regardless of our (in)ability to process output. Since there is no way to determine if a CLI command is meant to retrieve information or manipulate it, we have to assume that it is manipulating something. So I agree that descriptive error messages are useful, we need to avoid lying to the caller by claiming that the command failed to run if it did run.
For times where people are just retrieving information, couldn't we respond with success, put the error in 'Message:', and provide no 'Output:' headers? This way if you care about the output, you can detect the lack of output.
This is a tricky one, but I think I agree with Corey on this.
We will already send an Error with a Message value if the command was executed but returned a failure response code. The fact that we executed the command successfully but had an internal error that precluded showing the response feels like it should be different from conditions where we either never ran the command at all.
How about changing the astman_send_error to the Message: that is sent with a status of Error? In those cases, there wouldn't be any response that follows - since we can't send it - but there is still an overall indication that the command succeeded (sorta).
My concern with indicating 'Success' on failed commands is that the caller then needs to check the output for a known string to determine if the command executed. That would be different for every command.

I am fine with putting the error in the Message: header. Whether the command actually succeeded (Message: Command executed) can be put there as well.

Perhaps it would be easier to agree on the output with a few examples, this is the proposed behaviour:

1. Command NOT executed:

Response: Error
Message: Failed to create temporary file: Read-only file system

2. Command NOT executed, output available (eg: No such command 'XXX'):

Response: Success
Message: Command failed
Output: <failure reason>

3. Command WAS executed, but output is unavailable due to an internal error:

Response: Success
Message: Failed to allocate memory

4. Command WAS executed, output available:

Response: Success
Message: Command executed
Output: <output from the command>

While we are here, the temporary file path is hard-coded to /tmp/ast-ami-XXXXXX, should that be AST_SPOOL_DIR/tmp/ast-ami-XXXXXX?


- gareth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15039
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 9, 2015, 5:05 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.
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".
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
Corey Farrell
2015-04-10 04:59:53 UTC
Permalink
Post by gareth
/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.
I do not feel this issue is resolved. If a command has side-effects, we should respond with Success/Failure based on if the command ran - regardless of our (in)ability to process output. Since there is no way to determine if a CLI command is meant to retrieve information or manipulate it, we have to assume that it is manipulating something. So I agree that descriptive error messages are useful, we need to avoid lying to the caller by claiming that the command failed to run if it did run.
For times where people are just retrieving information, couldn't we respond with success, put the error in 'Message:', and provide no 'Output:' headers? This way if you care about the output, you can detect the lack of output.
This is a tricky one, but I think I agree with Corey on this.
We will already send an Error with a Message value if the command was executed but returned a failure response code. The fact that we executed the command successfully but had an internal error that precluded showing the response feels like it should be different from conditions where we either never ran the command at all.
How about changing the astman_send_error to the Message: that is sent with a status of Error? In those cases, there wouldn't be any response that follows - since we can't send it - but there is still an overall indication that the command succeeded (sorta).
My concern with indicating 'Success' on failed commands is that the caller then needs to check the output for a known string to determine if the command executed. That would be different for every command.
I am fine with putting the error in the Message: header. Whether the command actually succeeded (Message: Command executed) can be put there as well.
Response: Error
Message: Failed to create temporary file: Read-only file system
Response: Success
Message: Command failed
Output: <failure reason>
Response: Success
Message: Failed to allocate memory
Response: Success
Message: Command executed
Output: <output from the command>
While we are here, the temporary file path is hard-coded to /tmp/ast-ami-XXXXXX, should that be AST_SPOOL_DIR/tmp/ast-ami-XXXXXX?
The only one I disagree with is #2. If the command was not executed, that should be "Response: Error".


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15039
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 9, 2015, 11:52 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.
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".
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
gareth
2015-04-09 05:05:26 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------

(Updated April 9, 2015, 5:05 p.m.)


Review request for Asterisk Developers.


Changes
-------

Address Corey's findings -

Include the actual reason command response construction failed in the Error response Message header.

Changed return value of ast_cli_command_full so that we can return an Error response if the command failed.


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 (updated)
-----

/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
Corey Farrell
2015-04-09 20:30:38 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15168
-----------------------------------------------------------



/trunk/main/cli.c
<https://reviewboard.asterisk.org/r/4391/#comment25830>

These two lines should be replaced with 'goto done;'. This way we will return RESULT_FAILURE.


- Corey Farrell
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 9, 2015, 1:05 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.
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".
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
gareth
2015-04-10 03:52:41 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------

(Updated April 10, 2015, 3:52 p.m.)


Review request for Asterisk Developers.


Changes
-------

Update patch to use new proposed behaviour.


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 (updated)
-----

/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
Corey Farrell
2015-04-10 04:22:44 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15174
-----------------------------------------------------------



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

This is where we need to run astman_send_response_full to start the response with "Success" or "Error" depending on the value of ret.


- Corey Farrell
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 9, 2015, 11:52 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.
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".
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
gareth
2015-04-13 05:00:41 UTC
Permalink
-----------------------------------------------------------
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.


Changes
-------

Changed behaviour so that "Response: Error" is now sent if execution of the command fails.


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 (updated)
-----

/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
Corey Farrell
2015-04-13 07:41:44 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15190
-----------------------------------------------------------


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/


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

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.


- Corey Farrell
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 13, 2015, 1 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.
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".
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
Matt Jordan
2015-04-14 02:44:01 UTC
Permalink
Post by gareth
Post by Corey Farrell
* 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/
I'd be willing to take on the starpy issues if/when this gets pulled over.


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/#review15190
-----------------------------------------------------------
Post by gareth
-----------------------------------------------------------
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------
(Updated April 13, 2015, midnight)
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.
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".
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
gareth
2015-04-14 05:08:01 UTC
Permalink
Post by Matt Jordan
Post by Corey Farrell
* 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/
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.
Post by Matt Jordan
Post by Corey Farrell
/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
-----------------------------------------------------------
Post by Matt Jordan
-----------------------------------------------------------
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.
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".
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
gareth
2015-04-17 03:45:44 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4391/
-----------------------------------------------------------

(Updated April 17, 2015, 3:45 p.m.)


Status
------

This change has been discarded.


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

Loading...