[asterisk-dev] [Code Review] 4023: Allow passing options and command to MixMonitor when recording in ConfBridge
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4023/
---
Review request for Asterisk Developers.
Bugs: ASTERISK-24351
https://issues.asterisk.org/jira/browse/ASTERISK-24351
Repository: Asterisk
Description
---
This patch adds the ability to pass options and a command to MixMontor when
recording a conference using ConfBridge.
New options are -
record_options: Options to MixMontor, eg: m(), W() etc.
record_command: The command to execute when recording is over.
eg: Set(CONFBRIDGE(bridge,record_command)=/path/to/command
^{MIXMONITOR_FILENAME}))
Diffs
-
/trunk/configs/samples/confbridge.conf.sample 423782
/trunk/apps/confbridge/include/confbridge.h 423782
/trunk/apps/confbridge/conf_config_parser.c 423782
/trunk/apps/app_confbridge.c 423782
Diff: https://reviewboard.asterisk.org/r/4023/diff/
Testing
---
Set record_options to m(${MAILBOX}) and verified that a recording was delivered
to ${MAILBOX}.
Set record_command to /bin/rm ^{MIXMONITOR_FILENAME} and checked that recording
was deleted on ending the conference.
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
Re: [asterisk-dev] [Code Review] 4023: Allow passing options and command to MixMonitor when recording in ConfBridge
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4023/
---
(Updated Sept. 26, 2014, 4:23 a.m.)
Review request for Asterisk Developers.
Changes
---
Updated description.
Bugs: ASTERISK-24351
https://issues.asterisk.org/jira/browse/ASTERISK-24351
Repository: Asterisk
Description (updated)
---
This patch adds the ability to pass options and a command to MixMontor when
recording a conference using ConfBridge.
New options are -
record_options: Options to MixMontor, eg: m(), W() etc.
record_command: The command to execute when recording is over.
eg: Set(CONFBRIDGE(bridge,record_command)=/path/to/command
^{MIXMONITOR_FILENAME}))
Note:
The current behavior of set_rec_filename is to always append a timestamp to
rec_file. This is desirable for dynamically generated rec_file, but doesn't
make sense for a user-supplied rec_file and the documenation does not mention
that it does this either.
So the patch changes set_rec_filename to use the user-supplied rec_file as-is.
Diffs
-
/trunk/configs/samples/confbridge.conf.sample 423782
/trunk/apps/confbridge/include/confbridge.h 423782
/trunk/apps/confbridge/conf_config_parser.c 423782
/trunk/apps/app_confbridge.c 423782
Diff: https://reviewboard.asterisk.org/r/4023/diff/
Testing
---
Set record_options to m(${MAILBOX}) and verified that a recording was delivered
to ${MAILBOX}.
Set record_command to /bin/rm ^{MIXMONITOR_FILENAME} and checked that recording
was deleted on ending the conference.
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
Re: [asterisk-dev] [Code Review] 4023: Allow passing options and command to MixMonitor when recording in ConfBridge
> On Sept. 26, 2014, 1:11 p.m., Corey Farrell wrote:
> > I believe this change would allow AMI users to execute arbitrary commands
> > on the Asterisk server (security risk). The ability to SetVar
> > CONFBRIDGE(bridge,record_command) from AMI would need to be blocked when
> > asterisk.conf has live_dangerously=no.
Agreed, also current write access to record_file allows a user to overwrite any
audio file Asterisk has write access to. eg: setting
CONFBRIDGE(brigde,record_file) to
${ASTSPOOLDIR}/voicemail/default/${MAILBOX}/busy.gsm.
- gareth
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4023/#review13385
---
On Sept. 26, 2014, 4:23 a.m., gareth wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4023/
> ---
>
> (Updated Sept. 26, 2014, 4:23 a.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-24351
> https://issues.asterisk.org/jira/browse/ASTERISK-24351
>
>
> Repository: Asterisk
>
>
> Description
> ---
>
> This patch adds the ability to pass options and a command to MixMontor when
> recording a conference using ConfBridge.
>
> New options are -
>
> record_options: Options to MixMontor, eg: m(), W() etc.
> record_command: The command to execute when recording is over.
>
> eg: Set(CONFBRIDGE(bridge,record_command)=/path/to/command
> ^{MIXMONITOR_FILENAME}))
>
> Note:
>
> The current behavior of set_rec_filename is to always append a timestamp to
> rec_file. This is desirable for dynamically generated rec_file, but doesn't
> make sense for a user-supplied rec_file and the documenation does not mention
> that it does this either.
>
> So the patch changes set_rec_filename to use the user-supplied rec_file
> as-is.
>
>
> Diffs
> -
>
> /trunk/configs/samples/confbridge.conf.sample 423782
> /trunk/apps/confbridge/include/confbridge.h 423782
> /trunk/apps/confbridge/conf_config_parser.c 423782
> /trunk/apps/app_confbridge.c 423782
>
> Diff: https://reviewboard.asterisk.org/r/4023/diff/
>
>
> Testing
> ---
>
> Set record_options to m(${MAILBOX}) and verified that a recording was
> delivered to ${MAILBOX}.
>
> Set record_command to /bin/rm ^{MIXMONITOR_FILENAME} and checked that
> recording was deleted on ending the conference.
>
>
> 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
Re: [asterisk-dev] [Code Review] 4023: Allow passing options and command to MixMonitor when recording in ConfBridge
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4023/
---
(Updated Sept. 27, 2014, 6:30 a.m.)
Review request for Asterisk Developers.
Changes
---
Updated patch to address Corey's comments.
CONFBRIDGE() is now registered as an escalating write function.
Bugs: ASTERISK-24351
https://issues.asterisk.org/jira/browse/ASTERISK-24351
Repository: Asterisk
Description
---
This patch adds the ability to pass options and a command to MixMontor when
recording a conference using ConfBridge.
New options are -
record_options: Options to MixMontor, eg: m(), W() etc.
record_command: The command to execute when recording is over.
eg: Set(CONFBRIDGE(bridge,record_command)=/path/to/command
^{MIXMONITOR_FILENAME}))
Note:
The current behavior of set_rec_filename is to always append a timestamp to
rec_file. This is desirable for dynamically generated rec_file, but doesn't
make sense for a user-supplied rec_file and the documenation does not mention
that it does this either.
So the patch changes set_rec_filename to use the user-supplied rec_file as-is.
Diffs (updated)
-
/trunk/configs/samples/confbridge.conf.sample 424055
/trunk/apps/confbridge/include/confbridge.h 424055
/trunk/apps/confbridge/conf_config_parser.c 424055
/trunk/apps/app_confbridge.c 424055
Diff: https://reviewboard.asterisk.org/r/4023/diff/
Testing
---
Set record_options to m(${MAILBOX}) and verified that a recording was delivered
to ${MAILBOX}.
Set record_command to /bin/rm ^{MIXMONITOR_FILENAME} and checked that recording
was deleted on ending the conference.
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
[asterisk-dev] [Code Review] 4050: Add ability for Channel Drivers to provide Presence State information
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4050/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24363 https://issues.asterisk.org/jira/browse/ASTERISK-24363 Repository: Asterisk Description --- This patch adds the ability for channel drivers to supply presence information in a similar manner to device state. eg: exten => XXX,hint,,/ Diffs - /trunk/main/presencestate.c 424055 /trunk/main/channel.c 424055 /trunk/include/asterisk/channel.h 424055 Diff: https://reviewboard.asterisk.org/r/4050/diff/ Testing --- Code is originally written as part of ASTERISK-13145 which has undergone extensive testing. 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
Re: [asterisk-dev] [Code Review] 4050: Add ability for Channel Drivers to provide Presence State information
> On Oct. 14, 2014, 1:51 p.m., Matt Jordan wrote: > > /trunk/main/presencestate.c, lines 160-177 > > <https://reviewboard.asterisk.org/r/4050/diff/1/?file=67849#file67849line160> > > > > So, I'm not sure this is the behaviour that we would want. > > > > If a channel driver provides presence information, then the presence > > information provided by the channel driver supercedes any custom presence > > provider. That is: > > > > exten => hint,SIP/alice&CustomPresence:alice > > > > Will *always* use the presence provided by SIP/alice, instead of the > > custom presence provider. That feels like a loss of functionality. > > > > Prior to this patch, we would only use the presence information > > provided by a single custom presence provider, since presence information > > could only come from custom presence providers. If the channel drivers > > provide that as well, there probably needs to be some mechanism to > > aggregate that together. Supporting multiple providers would be better. It turned out this wasn't just a matter of putting a strsep() loop in ast_presence_state_helper(). This hint parsing code in presence_state_cb (main/pbx.c) only supported a single provider, so I had to modify add_hintdevice() to include the presence hints and then rewrote the lookup code based on that in device_state_cb(). The overall presence state is whichever provider is most-unavailable, eg: if SIP/alice is DND and CustomPresence:alice is CHAT then the presence state is DND. - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4050/#review13510 --- On Oct. 6, 2014, 4:18 a.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4050/ > --- > > (Updated Oct. 6, 2014, 4:18 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24363 > https://issues.asterisk.org/jira/browse/ASTERISK-24363 > > > Repository: Asterisk > > > Description > --- > > This patch adds the ability for channel drivers to supply presence > information in a similar manner to device state. > > eg: exten => XXX,hint,,/ > > > Diffs > - > > /trunk/main/presencestate.c 424055 > /trunk/main/channel.c 424055 > /trunk/include/asterisk/channel.h 424055 > > Diff: https://reviewboard.asterisk.org/r/4050/diff/ > > > Testing > --- > > Code is originally written as part of ASTERISK-13145 which has undergone > extensive testing. > > > 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
Re: [asterisk-dev] [Code Review] 4050: Add ability for Channel Drivers to provide Presence State information
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4050/ --- (Updated Oct. 17, 2014, 4:26 a.m.) Review request for Asterisk Developers. Changes --- Updated patch to support multiple presence providers as per Matt's request. Bugs: ASTERISK-24363 https://issues.asterisk.org/jira/browse/ASTERISK-24363 Repository: Asterisk Description --- This patch adds the ability for channel drivers to supply presence information in a similar manner to device state. eg: exten => XXX,hint,,/ Diffs (updated) - /trunk/main/presencestate.c 425756 /trunk/main/pbx.c 425756 /trunk/main/channel.c 425756 /trunk/include/asterisk/channel.h 425756 Diff: https://reviewboard.asterisk.org/r/4050/diff/ Testing --- Code is originally written as part of ASTERISK-13145 which has undergone extensive testing. 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
Re: [asterisk-dev] [Code Review] 4050: Add ability for Channel Drivers to provide Presence State information
On Dec. 1, 2014, 11:46 p.m., gareth wrote: > > This could probably use a unit test, especially since you didn't add any > > support for this feature to any of the existing channel drivers in this > > patch. > > > > I would imagine a unit test to do the following: > > > > 1 - Create a channel tech with a presence_provider function (see > > res/parking/parking_tests for an example of a temporary tech just used for > > a unit test) > > 2 - Create a channel using that tech > > 3 - Use your presence provider function to retrieve the presence state of > > the channel > > 4 - Change the presence state of the channel > > 5 - Check presence state again > > 6 - release the channel I will add a unit test. - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4050/#review13858 --- On Oct. 17, 2014, 4:26 a.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4050/ > --- > > (Updated Oct. 17, 2014, 4:26 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24363 > https://issues.asterisk.org/jira/browse/ASTERISK-24363 > > > Repository: Asterisk > > > Description > --- > > This patch adds the ability for channel drivers to supply presence > information in a similar manner to device state. > > eg: exten => XXX,hint,,/ > > > Diffs > - > > /trunk/main/presencestate.c 425756 > /trunk/main/pbx.c 425756 > /trunk/main/channel.c 425756 > /trunk/include/asterisk/channel.h 425756 > > Diff: https://reviewboard.asterisk.org/r/4050/diff/ > > > Testing > --- > > Code is originally written as part of ASTERISK-13145 which has undergone > extensive testing. > > > 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
Re: [asterisk-dev] [Code Review] 4050: Add ability for Channel Drivers to provide Presence State information
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4050/ --- (Updated Dec. 3, 2014, 4:32 a.m.) Review request for Asterisk Developers. Changes --- Updated patch to address Jonathan's comments. Remove erroneous #include. Added unit test. Bugs: ASTERISK-24363 https://issues.asterisk.org/jira/browse/ASTERISK-24363 Repository: Asterisk Description --- This patch adds the ability for channel drivers to supply presence information in a similar manner to device state. eg: exten => XXX,hint,,/ Diffs (updated) - /trunk/main/presencestate.c 428167 /trunk/main/pbx.c 428167 /trunk/main/channel.c 428167 /trunk/include/asterisk/channel.h 428167 Diff: https://reviewboard.asterisk.org/r/4050/diff/ Testing --- Code is originally written as part of ASTERISK-13145 which has undergone extensive testing. 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
Re: [asterisk-dev] [Code Review] 4023: Allow passing options and command to MixMonitor when recording in ConfBridge
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4023/
---
(Updated Dec. 3, 2014, 9:37 a.m.)
Review request for Asterisk Developers.
Changes
---
Updated Group, as the MixMonitor vulnerability discovered via this patch was
fixed in 11.14.1/12.7.1/13.0.1
Bugs: ASTERISK-24351
https://issues.asterisk.org/jira/browse/ASTERISK-24351
Repository: Asterisk
Description
---
This patch adds the ability to pass options and a command to MixMontor when
recording a conference using ConfBridge.
New options are -
record_options: Options to MixMontor, eg: m(), W() etc.
record_command: The command to execute when recording is over.
eg: Set(CONFBRIDGE(bridge,record_command)=/path/to/command
^{MIXMONITOR_FILENAME}))
Note:
The current behavior of set_rec_filename is to always append a timestamp to
rec_file. This is desirable for dynamically generated rec_file, but doesn't
make sense for a user-supplied rec_file and the documenation does not mention
that it does this either.
So the patch changes set_rec_filename to use the user-supplied rec_file as-is.
Diffs
-
/trunk/configs/samples/confbridge.conf.sample 428862
/trunk/apps/confbridge/include/confbridge.h 428862
/trunk/apps/confbridge/conf_config_parser.c 428862
/trunk/apps/app_confbridge.c 428862
Diff: https://reviewboard.asterisk.org/r/4023/diff/
Testing
---
Set record_options to m(${MAILBOX}) and verified that a recording was delivered
to ${MAILBOX}.
Set record_command to /bin/rm ^{MIXMONITOR_FILENAME} and checked that recording
was deleted on ending the conference.
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
Re: [asterisk-dev] [Code Review] 4050: Add ability for Channel Drivers to provide Presence State information
> On Dec. 1, 2014, 11:46 p.m., Jonathan Rose wrote: > > /trunk/main/pbx.c, lines 11874-11877 > > <https://reviewboard.asterisk.org/r/4050/diff/2/?file=68397#file68397line11874> > > > > Consider an assertion here? That part of the code (and it's comment) were copied from device_state_cb(). The NULL check may be unnecessary now as modification to hintdevices is protected by context_merge_lock, so device->hint can't be set to NULL by hintdevice_destroy during a dialplan reload. I could remove the check from both callbacks if required. - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4050/#review13858 --- On Dec. 3, 2014, 4:32 a.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4050/ > --- > > (Updated Dec. 3, 2014, 4:32 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24363 > https://issues.asterisk.org/jira/browse/ASTERISK-24363 > > > Repository: Asterisk > > > Description > --- > > This patch adds the ability for channel drivers to supply presence > information in a similar manner to device state. > > eg: exten => XXX,hint,,/ > > > Diffs > - > > /trunk/main/presencestate.c 428167 > /trunk/main/pbx.c 428167 > /trunk/main/channel.c 428167 > /trunk/include/asterisk/channel.h 428167 > > Diff: https://reviewboard.asterisk.org/r/4050/diff/ > > > Testing > --- > > Code is originally written as part of ASTERISK-13145 which has undergone > extensive testing. > > > 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
Re: [asterisk-dev] [Code Review] 4023: Allow passing options and command to MixMonitor when recording in ConfBridge
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4023/
---
(Updated Dec. 21, 2014, 8:35 p.m.)
Status
--
This change has been marked as submitted.
Review request for Asterisk Developers.
Changes
---
Committed in revision 429934
Bugs: ASTERISK-24351
https://issues.asterisk.org/jira/browse/ASTERISK-24351
Repository: Asterisk
Description
---
This patch adds the ability to pass options and a command to MixMontor when
recording a conference using ConfBridge.
New options are -
record_options: Options to MixMontor, eg: m(), W() etc.
record_command: The command to execute when recording is over.
eg: Set(CONFBRIDGE(bridge,record_command)=/path/to/command
^{MIXMONITOR_FILENAME}))
Note:
The current behavior of set_rec_filename is to always append a timestamp to
rec_file. This is desirable for dynamically generated rec_file, but doesn't
make sense for a user-supplied rec_file and the documenation does not mention
that it does this either.
So the patch changes set_rec_filename to use the user-supplied rec_file as-is.
Diffs
-
/trunk/configs/samples/confbridge.conf.sample 428862
/trunk/apps/confbridge/include/confbridge.h 428862
/trunk/apps/confbridge/conf_config_parser.c 428862
/trunk/apps/app_confbridge.c 428862
Diff: https://reviewboard.asterisk.org/r/4023/diff/
Testing
---
Set record_options to m(${MAILBOX}) and verified that a recording was delivered
to ${MAILBOX}.
Set record_command to /bin/rm ^{MIXMONITOR_FILENAME} and checked that recording
was deleted on ending the conference.
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
Re: [asterisk-dev] [Code Review] 4050: Add ability for Channel Drivers to provide Presence State information
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4050/ --- (Updated Dec. 22, 2014, 8:33 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 429967 Bugs: ASTERISK-24363 https://issues.asterisk.org/jira/browse/ASTERISK-24363 Repository: Asterisk Description --- This patch adds the ability for channel drivers to supply presence information in a similar manner to device state. eg: exten => XXX,hint,,/ Diffs - /trunk/main/presencestate.c 428167 /trunk/main/pbx.c 428167 /trunk/main/channel.c 428167 /trunk/include/asterisk/channel.h 428167 Diff: https://reviewboard.asterisk.org/r/4050/diff/ Testing --- Code is originally written as part of ASTERISK-13145 which has undergone extensive testing. 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
[asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
> On Jan. 30, 2015, 1:22 a.m., George Joseph wrote: > > 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 ------- On Jan. 30, 2015, 12:25 a.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > 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. > > 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 > > -- _ -- 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
Re: [asterisk-dev] [Code Review] 4473: chan_pjsip: Add "rpid_immediate" option to prevent unnecessary "180 Ringing" messages.
> On March 12, 2015, 8:41 p.m., Matt Jordan wrote: > > 1. For this to go into Asterisk 13, tests will need to be provided that > > cover the new parameter. (Really, those tests should be written regardless) > > 2. The CHANGES file will need to get updated with the new option. > > rmudgett wrote: > Actually I'd prefer that the rpid_immediate option not exist at all and > the code it controls to just be removed. Sending connected line updates back > to the caller _before_ getting connected doesn't really make sense. This is > what the REDIRECTING information is supposed to be doing. I disagree, providing connected line updates pre-answer makes perfect sense. Why would I want to know the connected-line name only after the call has been answered? That assumes the call even is answered, sending the connected-line information immediately allows the phone to include the name in it's call history. If no call-forwarding and/or re-addressing has taken place why would REDIRECTING be used? - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14669 --- On March 10, 2015, 11:48 p.m., rmudgett wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4473/ > --- > > (Updated March 10, 2015, 11:48 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24781 > https://issues.asterisk.org/jira/browse/ASTERISK-24781 > > > Repository: Asterisk > > > Description > --- > > This patch builds on https://reviewboard.asterisk.org/r/4472/ > > When that patch is committed this patch reduces to simply adding the > rpid_immediate option to skip generating INVITE response messages as a > result of connected line updates. > > > Incoming PJSIP call legs that have not been answered yet send unnecessary > "180 Ringing" or "183 Progress" messages every time a connected line > update happens. If the outgoing channel is also PJSIP then the incoming > channel will always send a "180 Ringing" or "183 Progress" message when > the outgoing channel sends the INVITE. > > Consequences of these unnecessary messages: > > * The caller can start hearing ringback before the far end even gets the > call. > > * Many phones tend to grab the first connected line information and refuse > to update the display if it changes. The first information is not likely > to be correct if the call goes to an endpoint not under the control of the > first Asterisk box. > > When connected line first went into Asterisk in v1.8, chan_sip received an > undocumented option "rpid_immediate" that defaults to disabled. When > enabled, the option immediately passes connected line update information > to the caller in "180 Ringing" or "183 Progress" messages as described > above. > > * Added "rpid_immediate" option to prevent unnecessary "180 Ringing" or > "183 Progress" messages. The default is "no" to disable sending the > unnecessary messages. > > > Diffs > - > > /branches/13/res/res_pjsip_caller_id.c 432761 > /branches/13/res/res_pjsip/pjsip_configuration.c 432761 > /branches/13/res/res_pjsip.c 432761 > /branches/13/include/asterisk/res_pjsip.h 432761 > /branches/13/configs/samples/pjsip.conf.sample 432761 > /branches/13/channels/chan_pjsip.c 432761 > > Diff: https://reviewboard.asterisk.org/r/4473/diff/ > > > Testing > --- > > * Ran the tests/channels/pjsip testsuite tests. They still pass. > > * Made a call chain as follows: 100 -> * -> * -> * -> 200. With the patch > there are no unnecessary messages. Without the patch there were several > "180 Ringing" messages sent back to the caller. > > > Thanks, > > rmudgett > > -- _ -- 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
Re: [asterisk-dev] [Code Review] 4473: chan_pjsip: Add "rpid_immediate" option to prevent unnecessary "180 Ringing" messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14690 --- Ship it! Ship It! - gareth On March 13, 2015, 5:13 p.m., rmudgett wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4473/ > --- > > (Updated March 13, 2015, 5:13 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24781 > https://issues.asterisk.org/jira/browse/ASTERISK-24781 > > > Repository: Asterisk > > > Description > --- > > Incoming PJSIP call legs that have not been answered yet send unnecessary > "180 Ringing" or "183 Progress" messages every time a connected line > update happens. If the outgoing channel is also PJSIP then the incoming > channel will always send a "180 Ringing" or "183 Progress" message when > the outgoing channel sends the INVITE. > > Consequences of these unnecessary messages: > > * The caller can start hearing ringback before the far end even gets the > call. > > * Many phones tend to grab the first connected line information and refuse > to update the display if it changes. The first information is not likely > to be correct if the call goes to an endpoint not under the control of the > first Asterisk box. > > When connected line first went into Asterisk in v1.8, chan_sip received an > undocumented option "rpid_immediate" that defaults to disabled. When > enabled, the option immediately passes connected line update information > to the caller in "180 Ringing" or "183 Progress" messages as described > above. > > * Added "rpid_immediate" option to prevent unnecessary "180 Ringing" or > "183 Progress" messages. The default is "no" to disable sending the > unnecessary messages. > > > Diffs > - > > /branches/13/res/res_pjsip/pjsip_configuration.c 432895 > /branches/13/res/res_pjsip.c 432895 > /branches/13/include/asterisk/res_pjsip.h 432895 > /branches/13/configs/samples/pjsip.conf.sample 432895 > /branches/13/channels/chan_pjsip.c 432895 > > Diff: https://reviewboard.asterisk.org/r/4473/diff/ > > > Testing > --- > > * Ran the tests/channels/pjsip testsuite tests. They still pass. > > * Made a call chain as follows: 100 -> * -> * -> * -> 200. With the patch > there are no unnecessary messages. Without the patch there were several > "180 Ringing" messages sent back to the caller. > > > Thanks, > > rmudgett > > -- _ -- 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
Re: [asterisk-dev] [Code Review] 4473: chan_pjsip: Add "rpid_immediate" option to prevent unnecessary "180 Ringing" messages.
> On March 12, 2015, 8:41 p.m., Matt Jordan wrote: > > 1. For this to go into Asterisk 13, tests will need to be provided that > > cover the new parameter. (Really, those tests should be written regardless) > > 2. The CHANGES file will need to get updated with the new option. > > rmudgett wrote: > Actually I'd prefer that the rpid_immediate option not exist at all and > the code it controls to just be removed. Sending connected line updates back > to the caller _before_ getting connected doesn't really make sense. This is > what the REDIRECTING information is supposed to be doing. > > gareth wrote: > I disagree, providing connected line updates pre-answer makes perfect > sense. Why would I want to know the connected-line name only after the call > has been answered? > > That assumes the call even is answered, sending the connected-line > information immediately allows the phone to include the name in it's call > history. > > If no call-forwarding and/or re-addressing has taken place why would > REDIRECTING be used? > > rmudgett wrote: > OK, that makes sense but the code that rpid_immediate controls doesn't. > As described in the review description, that code immediately sends redundant > "180 Ringing" or "183 Progress" messages at best and at worst lies that the > other end is ringing with inaccurate connected line information. > > Matt Jordan wrote: > I'm not sure what this setting is attempting to do then. The description > of the new setting makes it sound like it controls sending connected line > information prior to Answer, which fits gareth's interpretation. Whether or > not it sends multiple 180/183 provisional responses does not seem to be > implied in the description of the setting. > > If setting 'rpid_immediate' to 'yes' still results in extraneous 180/183 > provisional responses, then I'm not sure this patch fixes the underlying > issue. > > rmudgett wrote: > The option does exactly the same thing as chan_sip and will be disabled > by default just like chan_sip. > > When disabled it does not send the extraneous 180/183 messages. The > connected line information simply waits until there is a real reason to send > the 180/183 messages. e.g., Such as when the outgoing channel receives a > 180/183 message. When the outgoing channel receives a 180/183 message, that > message may also contain updated/correct connected line information from the > peer. > > When enabled it sends the connected line update information *immediately* > on a 180/183 message regardless of if the far end has received the call yet > or the connected line information came from the far end. The core may do > some filtering to eliminate an update event if the connected line information > does not change. However, when the peer receives a 180/183 message the > caller will also get the 180/183 message generated because of the > ast_indicate(AST_CONTROL_RINGING/AST_CONTROL_PROGRESS). > > The key word is immediate. I suppose I should sprinkle immediate a few > more times in the option description. I doubt anyone even enables the option > in chan_sip because it is undocumented, disabled by default, and will cause > this odd behaviour when enabled. > > These are the reasons why I think the code the option controls should > just be removed and the option not even exist. The code doesn't really help > and the extraneous 180/183 messages cause problems when enabled. I have rpid_immediate enabled. I know about the option because I searched through chan_sip.c to find out why RPID updates were not working as expected. Until the SIP protocol gains a "here is the new RPID info" response there is always going to be the problem of piggy-backing that information on a response which already had a meaning. Patch looks fine. - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4473/#review14669 --- On March 13, 2015, 5:13 p.m., rmudgett wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4473/ > --- > > (Updated March 13, 2015, 5:13 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24781 > https://issues.asterisk.org/jira/browse/ASTERISK-24781 > > > Repository: Asterisk > > >
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
> On Jan. 30, 2015, 1:22 a.m., George Joseph wrote: > > If you run the Testsuite, you'll get a good indication of whether this is > > truly backwards compatible. > > gareth wrote: > 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". > > Corey Farrell wrote: > 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: > 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 --- On Jan. 30, 2015, 12:25 a.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > 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. > > 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 > > -- _ -- 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
> On Jan. 30, 2015, 1:22 a.m., George Joseph wrote: > > If you run the Testsuite, you'll get a good indication of whether this is > > truly backwards compatible. > > gareth wrote: > 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". > > Corey Farrell wrote: > 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: > 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. > > gareth wrote: > 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 > > Corey Farrell wrote: > 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. > > Matt Jordan wrote: > 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. 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 --- On Jan. 30, 2015, 12:25 a.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > 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. > > 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:
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- 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 -- _ -- 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
[asterisk-dev] [Code Review] 4515: build_peer peer mailbox management bug
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4515/ --- Review request for Asterisk Developers. Bugs: ASTERISK-24871 https://issues.asterisk.org/jira/browse/ASTERISK-24871 Repository: Asterisk Description --- During a reload, build_peer iterates over the peer's mailboxes and tags them for removal via the delme variable. It adds any new, unique mailboxes to the peer via add_peer_mailboxes and then removes any mailboxes with delme still set. However, there isn't any code to unset delme, so this would remove any previously configured mailboxes. That is not what happens though because build_peer also calls set_peer_defaults which clears out all of the configured mailboxes using clear_peer_mailboxes making the setting of delme redundant. So in the end there is no impact to the user because all the configured mailboxes get added regardless. Patch unsets delme for existing, still-configured mailboxes in add_peer_mailboxes and removes call to clear_peer_mailboxes. Diffs - /trunk/channels/chan_sip.c 433198 Diff: https://reviewboard.asterisk.org/r/4515/diff/ Testing --- Added new mailboxes to peer, reloaded chan_sip and verified that existing mailboxes were still there and new mailboxes had been added. Removed mailboxes from peer, reloaded chan_sip and verified that those mailboxes were no longer assigned to peer. 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
Re: [asterisk-dev] [Code Review] 4515: build_peer peer mailbox management bug
> On March 20, 2015, 4:40 p.m., Corey Farrell wrote: > > /trunk/channels/chan_sip.c, line 30264 > > <https://reviewboard.asterisk.org/r/4515/diff/1/?file=72691#file72691line30264> > > > > This could result in a change of behaviour. In build_peer 'first_pass' > > will no longer decide if the existing mailboxes get cleared, it would only > > be done when '!dev_state'. > > > > I feel that a safer change would be to just remove the delme variable, > > let the current logic live on. I could be convinced otherwise if you can > > show that the current logic produces the wrong results, but even then we'd > > have to very careful. My understanding of the code is that build_peer is only called with devstate_only=true for realtime peers not already cached in the peers table (rtcachefriends=yes) and the state of the peer is being checked via sip_devicestate [1]. So firstpass=false should only happen for realtime peers that had been semi-built (peer exists and peer->the_mark=false) as the result of a sip_devicestate call. [1] sip_unregister also calls with devstate_only=true, but it also sets realtime=false so realtime_peer will not be called. - gareth --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4515/#review14755 --- On March 20, 2015, 4:02 p.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4515/ > --- > > (Updated March 20, 2015, 4:02 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24871 > https://issues.asterisk.org/jira/browse/ASTERISK-24871 > > > Repository: Asterisk > > > Description > --- > > During a reload, build_peer iterates over the peer's mailboxes and tags them > for removal via the delme variable. It adds any new, unique mailboxes to the > peer via add_peer_mailboxes and then removes any mailboxes with delme still > set. > > However, there isn't any code to unset delme, so this would remove any > previously configured mailboxes. > > That is not what happens though because build_peer also calls > set_peer_defaults which clears out all of the configured mailboxes using > clear_peer_mailboxes making the setting of delme redundant. > > So in the end there is no impact to the user because all the configured > mailboxes get added regardless. > > Patch unsets delme for existing, still-configured mailboxes in > add_peer_mailboxes and removes call to clear_peer_mailboxes. > > > Diffs > - > > /trunk/channels/chan_sip.c 433198 > > Diff: https://reviewboard.asterisk.org/r/4515/diff/ > > > Testing > --- > > Added new mailboxes to peer, reloaded chan_sip and verified that existing > mailboxes were still there and new mailboxes had been added. > > Removed mailboxes from peer, reloaded chan_sip and verified that those > mailboxes were no longer assigned to peer. > > > 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
> On March 25, 2015, 9:24 a.m., Corey Farrell wrote:
> > /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.
> On March 25, 2015, 9:24 a.m., Corey Farrell wrote:
> > /trunk/main/manager.c, lines 4914-4916
> > <https://reviewboard.asterisk.org/r/4391/diff/2/?file=72690#file72690line4914>
> >
> > 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.
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
---
On March 20, 2015, 3:34 p.m., gareth wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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.
>
> 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
> 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 ' 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- 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 -- _ -- 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
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
> On April 13, 2015, 7:41 p.m., Corey Farrell wrote: > > So I think this looks pretty good. Next steps: > > * We've migrated to git. Take a look at [1] for information on how to use > > gerrit to post a git review. Don't worry you won't be facing the full > > review again, we've already dealt with the code issues. > > * We need to prepare a patch for starpy [2]. I don't have time to work on > > this right now. Do you know python or know someone who does and has > > time/willingness to help? > > * Verify the full testsuite passes against the latest Asterisk with this > > change. > > * Once starpy supports the change we will coordinate with Digium to ensure > > starpy is updated before we commit this to trunk. > > > > I know this seems like a lot, but we are changing the protocol, so we need > > to make sure we do not break the testsuite. > > > > [1] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage > > [2] https://github.com/asterisk/starpy/ > > Matt Jordan wrote: > I'd be willing to take on the starpy issues if/when this gets pulled over. I wouldn't claim to know python, but I have sent a pull request on github that changes starpy to use the new format. The testsuite only appears to use the command action in the apps/queues/set_penalty test and it doesn't even parse the output. The LUA AMI library in the testsuite checks for 'Response: Follows' so it shouldn't be affected by the new output format. > On April 13, 2015, 7:41 p.m., Corey Farrell wrote: > > /trunk/main/manager.c, line 4904 > > <https://reviewboard.asterisk.org/r/4391/diff/6/?file=73873#file73873line4904> > > > > Nit pick: "Success" or "Error" is already provided through a standard > > field, so this could be static - "Message: Command Output Follows\r\n". > > This would give a single value for Message that can be matched to determine > > that we successfully processed output, even if there are 0 lines or if the > > command provided the reason for failure in output. I will change this when I post the review to gerrit. - gareth ------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/#review15190 --- On April 13, 2015, 5 p.m., gareth wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4391/ > --- > > (Updated April 13, 2015, 5 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24730 > https://issues.asterisk.org/jira/browse/ASTERISK-24730 > > > Repository: Asterisk > > > Description > --- > > This patch adds a blank line between the headers and the output in the > Command action response. The reason for this change is to make it easier to > determine where the headers end and the output from the command starts. > > Currently, to parse a response to a Command action, one has to: > > 1. Read one line, if line is "Response: Error", parse the remaining as a > standard AMI response and stop. > 2. Read one more line - or two if you used an ActionID - those lines are the > headers. > 3. Then read everything up to "--END COMMAND--\r\n\r\n". > > That could be changed to: > > 1. Read standard AMI response. > 2. If "Response: Follows", then read everything up to "--END > COMMAND--\r\n\r\n". > > The AMI version has been increased to 2.8.0 as this is a protocol change and > so that clients detect the new behavior. > > Adding a blank line should not cause older parsers to fail as they have to > read everything up to "--END COMMAND--\r\n\r\n" anyway. > > Adding a blank line will also not cause the AMI to HTML/XML encoder to fail > to separate the headers from the output as it checks for the presence of a > ":" character, which a blank line does not contain. > > > Diffs > - > > /trunk/main/manager.c 434448 > /trunk/main/cli.c 434448 > /trunk/include/asterisk/manager.h 434448 > /trunk/UPGRADE.txt 434448 > > Diff: https://reviewboard.asterisk.org/r/4391/diff/ > > > Testing > --- > > Connected to manager, issued 'core show uptime' command and verified that > there was a blank line between the headers and output. > > > Thanks, > > gareth > > -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- 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 -- _ -- 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
