Re: [asterisk-dev] [Code Review] 4431: Increase WebSocket frame size and improve large read handling
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4431/ --- (Updated Feb. 25, 2015, 2:46 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 432236 Repository: Asterisk Description --- Some WebSocket applications, like [chan_respoke][], require a larger frame size than the default 8k; this patch bumps the default to 16k. This patch also fixes some problems exacerbated by large frames. The sanity counter was decremented on every fread attempt in ws_safe_read(), regardless of whether data was read from the socket or not. For large frames, this could result in loss of sanity prior to reading the entire frame. (16k frame / 1448 bytes per segment = 12 segments). This patch changes the sanity counter so that it only decrements when fread() doesn't read any bytes. This more closely matches the original intention of ws_safe_read(), given that the error message is "Websocket seems unresponsive". This patch also properly logs EOF conditions, so disconnects are no longer confused with unresponsive connections. [chan_respoke]: https://github.com/respoke/chan_respoke Diffs - /branches/11/res/res_http_websocket.c 431915 Diff: https://reviewboard.asterisk.org/r/4431/diff/ Testing --- Ran a Node app that continuously send large WebSocket frame to Asterisk. https://gist.github.com/leedm777/ba6d86468d7646073286 Without the patch, Asterisk fails in less than 10 frames. With the patch, it runs like a boss. Thanks, David Lee -- _ -- 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] 4431: Increase WebSocket frame size and improve large read handling
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4431/ --- (Updated Feb. 20, 2015, 9:39 a.m.) Review request for Asterisk Developers. Changes --- de-nit Repository: Asterisk Description --- Some WebSocket applications, like [chan_respoke][], require a larger frame size than the default 8k; this patch bumps the default to 16k. This patch also fixes some problems exacerbated by large frames. The sanity counter was decremented on every fread attempt in ws_safe_read(), regardless of whether data was read from the socket or not. For large frames, this could result in loss of sanity prior to reading the entire frame. (16k frame / 1448 bytes per segment = 12 segments). This patch changes the sanity counter so that it only decrements when fread() doesn't read any bytes. This more closely matches the original intention of ws_safe_read(), given that the error message is "Websocket seems unresponsive". This patch also properly logs EOF conditions, so disconnects are no longer confused with unresponsive connections. [chan_respoke]: https://github.com/respoke/chan_respoke Diffs (updated) - /branches/11/res/res_http_websocket.c 431915 Diff: https://reviewboard.asterisk.org/r/4431/diff/ Testing --- Ran a Node app that continuously send large WebSocket frame to Asterisk. https://gist.github.com/leedm777/ba6d86468d7646073286 Without the patch, Asterisk fails in less than 10 frames. With the patch, it runs like a boss. Thanks, David Lee -- _ -- 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] 4431: Increase WebSocket frame size and improve large read handling
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4431/ --- (Updated Feb. 19, 2015, 10:52 p.m.) Review request for Asterisk Developers. Changes --- Changed sanity check per review feedback. Repository: Asterisk Description --- Some WebSocket applications, like [chan_respoke][], require a larger frame size than the default 8k; this patch bumps the default to 16k. This patch also fixes some problems exacerbated by large frames. The sanity counter was decremented on every fread attempt in ws_safe_read(), regardless of whether data was read from the socket or not. For large frames, this could result in loss of sanity prior to reading the entire frame. (16k frame / 1448 bytes per segment = 12 segments). This patch changes the sanity counter so that it only decrements when fread() doesn't read any bytes. This more closely matches the original intention of ws_safe_read(), given that the error message is "Websocket seems unresponsive". This patch also properly logs EOF conditions, so disconnects are no longer confused with unresponsive connections. [chan_respoke]: https://github.com/respoke/chan_respoke Diffs (updated) - /branches/11/res/res_http_websocket.c 431915 Diff: https://reviewboard.asterisk.org/r/4431/diff/ Testing --- Ran a Node app that continuously send large WebSocket frame to Asterisk. https://gist.github.com/leedm777/ba6d86468d7646073286 Without the patch, Asterisk fails in less than 10 frames. With the patch, it runs like a boss. Thanks, David Lee -- _ -- 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] 4431: Increase WebSocket frame size and improve large read handling
> On Feb. 19, 2015, 7:58 p.m., rmudgett wrote:
> > /branches/11/res/res_http_websocket.c, line 386
> > <https://reviewboard.asterisk.org/r/4431/diff/1/?file=71469#file71469line386>
> >
> > Change to:
> > if (!--sanity) {
> > move after loop sanity check closing here.
> > }
> >
> > Moving the sanity exit handling here will avoid waiting (possibly for a
> > long time) for data when we have already failed sanity and it won't matter
> > if we somehow actually do read more data.
> >
> > The loop becomes:
> > sanity = 10;
> > for (;;) {
> > ...
> > if (!rlen) {
> > ...
> > if (!--sanity) {
> >sanity session close and exit.
> > }
> > }
> > }
> > return 0;
"possibly for a long time" is at most one second, right?
- David
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4431/#review14498
---
On Feb. 18, 2015, 4:32 p.m., David Lee wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4431/
> ---
>
> (Updated Feb. 18, 2015, 4:32 p.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Repository: Asterisk
>
>
> Description
> ---
>
> Some WebSocket applications, like [chan_respoke][], require a larger
> frame size than the default 8k; this patch bumps the default to 16k.
> This patch also fixes some problems exacerbated by large frames.
>
> The sanity counter was decremented on every fread attempt in
> ws_safe_read(), regardless of whether data was read from the socket or
> not. For large frames, this could result in loss of sanity prior to
> reading the entire frame. (16k frame / 1448 bytes per segment = 12
> segments).
>
> This patch changes the sanity counter so that it only decrements when
> fread() doesn't read any bytes. This more closely matches the original
> intention of ws_safe_read(), given that the error message is
> "Websocket seems unresponsive".
>
> This patch also properly logs EOF conditions, so disconnects are no
> longer confused with unresponsive connections.
>
> [chan_respoke]: https://github.com/respoke/chan_respoke
>
>
> Diffs
> -
>
> /branches/11/res/res_http_websocket.c 431915
>
> Diff: https://reviewboard.asterisk.org/r/4431/diff/
>
>
> Testing
> ---
>
> Ran a Node app that continuously send large WebSocket frame to Asterisk.
>
> https://gist.github.com/leedm777/ba6d86468d7646073286
>
> Without the patch, Asterisk fails in less than 10 frames. With the patch, it
> runs like a boss.
>
>
> Thanks,
>
> David Lee
>
>
--
_
-- 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] 4431: Increase WebSocket frame size and improve large read handling
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4431/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- Some WebSocket applications, like [chan_respoke][], require a larger frame size than the default 8k; this patch bumps the default to 16k. This patch also fixes some problems exacerbated by large frames. The sanity counter was decremented on every fread attempt in ws_safe_read(), regardless of whether data was read from the socket or not. For large frames, this could result in loss of sanity prior to reading the entire frame. (16k frame / 1448 bytes per segment = 12 segments). This patch changes the sanity counter so that it only decrements when fread() doesn't read any bytes. This more closely matches the original intention of ws_safe_read(), given that the error message is "Websocket seems unresponsive". This patch also properly logs EOF conditions, so disconnects are no longer confused with unresponsive connections. [chan_respoke]: https://github.com/respoke/chan_respoke Diffs - /branches/11/res/res_http_websocket.c 431915 Diff: https://reviewboard.asterisk.org/r/4431/diff/ Testing --- Ran a Node app that continuously send large WebSocket frame to Asterisk. https://gist.github.com/leedm777/ba6d86468d7646073286 Without the patch, Asterisk fails in less than 10 frames. With the patch, it runs like a boss. Thanks, David Lee -- _ -- 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] 4365: Adding AMQP backend for CDR and CEL
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/ --- (Updated Feb. 5, 2015, 7:13 p.m.) Review request for Asterisk Developers. Changes --- Addressed review feedback. * Used pre_apply_config * Fixed a doc typo Repository: Asterisk Description --- This patch adds an AMQP backend for both CDR and CEL. This allows these logs to be dispatched to a message broker, such as RabbitMQ, where they can reliably be queued for transmission to the handling application. The res_amqp module adds core AMQP protocol support. The provided API only supports publishing, since that's all that's needed for CDR/CEL. The AMQP feature of supporting multiple 'channels' per connection was also omitted. The underlying librabbitmq library is not thread safe, so the multiplexing benefits are limited. It also makes using the library more complicated, since clients have to worry about both connections and channels. The cdr_amqp module simply registers a CDR backend which encodes CDRs as JSON, and publishes them to an AMQP connection. The cel_amqp module similarly registers a CEL backend, which encodes CELs as JSON, and publishes them to an AMQP connection. Diffs (updated) - /branches/13/res/res_amqp.exports.in PRE-CREATION /branches/13/res/res_amqp.c PRE-CREATION /branches/13/res/amqp/internal.h PRE-CREATION /branches/13/res/amqp/config.c PRE-CREATION /branches/13/res/amqp/cli.c PRE-CREATION /branches/13/res/Makefile 431095 /branches/13/makeopts.in 431095 /branches/13/include/asterisk/cel.h 431095 /branches/13/include/asterisk/autoconfig.h.in 431095 /branches/13/include/asterisk/amqp.h PRE-CREATION /branches/13/contrib/scripts/install_prereq 431095 /branches/13/configure.ac 431095 /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/amqp.conf.sample PRE-CREATION /branches/13/cel/cel_amqp.c PRE-CREATION /branches/13/cdr/cdr_amqp.c PRE-CREATION /branches/13/build_tools/menuselect-deps.in 431095 Diff: https://reviewboard.asterisk.org/r/4365/diff/ Testing --- Ran amqp-consume to print messages sent to queues. Confirmed that the amqp test command sent messages properly. Confirmed that CDRs were properly sent. Confirmed that CELs were properly sent. Thanks, David Lee -- _ -- 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] 4365: Adding AMQP backend for CDR and CEL
> On Feb. 4, 2015, 8:21 a.m., Joshua Colp wrote: > > /branches/13/include/asterisk/cel.h, lines 37-38 > > <https://reviewboard.asterisk.org/r/4365/diff/3/?file=71072#file71072line37> > > > > Why were these added here? Both are used in this file. If you don't happen to #include them prior to cel.h in your .c file, it fails to compile. > On Feb. 4, 2015, 8:21 a.m., Joshua Colp wrote: > > /branches/13/res/amqp/config.c, lines 228-240 > > <https://reviewboard.asterisk.org/r/4365/diff/3/?file=71076#file71076line228> > > > > This should be done in the pre_apply_config callback so the > > configuration is completely validated before doing the atomic swap. > > > > As well - should connection validation failure cause the config to not > > get applied in that case? > should connection validation failure cause the config to not get applied in > that case? Yup. Fixed. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/#review14407 --- On Jan. 26, 2015, 9:16 a.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4365/ > --- > > (Updated Jan. 26, 2015, 9:16 a.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > --- > > This patch adds an AMQP backend for both CDR and CEL. This allows these > logs to be dispatched to a message broker, such as RabbitMQ, where they > can reliably be queued for transmission to the handling application. > > The res_amqp module adds core AMQP protocol support. The provided API > only supports publishing, since that's all that's needed for CDR/CEL. > The AMQP feature of supporting multiple 'channels' per connection was > also omitted. The underlying librabbitmq library is not thread safe, so > the multiplexing benefits are limited. It also makes using the library > more complicated, since clients have to worry about both connections and > channels. > > The cdr_amqp module simply registers a CDR backend which encodes CDRs as > JSON, and publishes them to an AMQP connection. > > The cel_amqp module similarly registers a CEL backend, which encodes > CELs as JSON, and publishes them to an AMQP connection. > > > Diffs > - > > /branches/13/res/res_amqp.exports.in PRE-CREATION > /branches/13/res/res_amqp.c PRE-CREATION > /branches/13/res/amqp/internal.h PRE-CREATION > /branches/13/res/amqp/config.c PRE-CREATION > /branches/13/res/amqp/cli.c PRE-CREATION > /branches/13/res/Makefile 431095 > /branches/13/makeopts.in 431095 > /branches/13/include/asterisk/cel.h 431095 > /branches/13/include/asterisk/autoconfig.h.in 431095 > /branches/13/include/asterisk/amqp.h PRE-CREATION > /branches/13/contrib/scripts/install_prereq 431095 > /branches/13/configure.ac 431095 > /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION > /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION > /branches/13/configs/samples/amqp.conf.sample PRE-CREATION > /branches/13/cel/cel_amqp.c PRE-CREATION > /branches/13/cdr/cdr_amqp.c PRE-CREATION > /branches/13/build_tools/menuselect-deps.in 431095 > > Diff: https://reviewboard.asterisk.org/r/4365/diff/ > > > Testing > --- > > Ran amqp-consume to print messages sent to queues. > > Confirmed that the amqp test command sent messages properly. > > Confirmed that CDRs were properly sent. > > Confirmed that CELs were properly sent. > > > Thanks, > > David Lee > > -- _ -- 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] 4365: Adding AMQP backend for CDR and CEL
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/ --- (Updated Jan. 26, 2015, 9:16 a.m.) Review request for Asterisk Developers. Changes --- Updated for latest 13 branch Repository: Asterisk Description --- This patch adds an AMQP backend for both CDR and CEL. This allows these logs to be dispatched to a message broker, such as RabbitMQ, where they can reliably be queued for transmission to the handling application. The res_amqp module adds core AMQP protocol support. The provided API only supports publishing, since that's all that's needed for CDR/CEL. The AMQP feature of supporting multiple 'channels' per connection was also omitted. The underlying librabbitmq library is not thread safe, so the multiplexing benefits are limited. It also makes using the library more complicated, since clients have to worry about both connections and channels. The cdr_amqp module simply registers a CDR backend which encodes CDRs as JSON, and publishes them to an AMQP connection. The cel_amqp module similarly registers a CEL backend, which encodes CELs as JSON, and publishes them to an AMQP connection. Diffs (updated) - /branches/13/res/res_amqp.exports.in PRE-CREATION /branches/13/res/res_amqp.c PRE-CREATION /branches/13/res/amqp/internal.h PRE-CREATION /branches/13/res/amqp/config.c PRE-CREATION /branches/13/res/amqp/cli.c PRE-CREATION /branches/13/res/Makefile 431095 /branches/13/makeopts.in 431095 /branches/13/include/asterisk/cel.h 431095 /branches/13/include/asterisk/autoconfig.h.in 431095 /branches/13/include/asterisk/amqp.h PRE-CREATION /branches/13/contrib/scripts/install_prereq 431095 /branches/13/configure.ac 431095 /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/amqp.conf.sample PRE-CREATION /branches/13/cel/cel_amqp.c PRE-CREATION /branches/13/cdr/cdr_amqp.c PRE-CREATION /branches/13/build_tools/menuselect-deps.in 431095 Diff: https://reviewboard.asterisk.org/r/4365/diff/ Testing --- Ran amqp-consume to print messages sent to queues. Confirmed that the amqp test command sent messages properly. Confirmed that CDRs were properly sent. Confirmed that CELs were properly sent. Thanks, David Lee -- _ -- 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] 4365: Adding AMQP backend for CDR and CEL
> On Jan. 21, 2015, 5:31 p.m., Mark Michelson wrote: > > /branches/13/cdr/cdr_amqp.c, lines 37-42 > > <https://reviewboard.asterisk.org/r/4365/diff/1/?file=70953#file70953line37> > > > > This needs to have details filled in. The actual doc is immediately below. This is an embarrassing vestigial copy/paste blob. > On Jan. 21, 2015, 5:31 p.m., Mark Michelson wrote: > > /branches/13/cel/cel_amqp.c, lines 37-42 > > <https://reviewboard.asterisk.org/r/4365/diff/1/?file=70954#file70954line37> > > > > This needs to be filled in. Another embarrassing vestigial copy/paste blob. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/#review14264 ------- On Jan. 23, 2015, 10:37 a.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4365/ > --- > > (Updated Jan. 23, 2015, 10:37 a.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > --- > > This patch adds an AMQP backend for both CDR and CEL. This allows these > logs to be dispatched to a message broker, such as RabbitMQ, where they > can reliably be queued for transmission to the handling application. > > The res_amqp module adds core AMQP protocol support. The provided API > only supports publishing, since that's all that's needed for CDR/CEL. > The AMQP feature of supporting multiple 'channels' per connection was > also omitted. The underlying librabbitmq library is not thread safe, so > the multiplexing benefits are limited. It also makes using the library > more complicated, since clients have to worry about both connections and > channels. > > The cdr_amqp module simply registers a CDR backend which encodes CDRs as > JSON, and publishes them to an AMQP connection. > > The cel_amqp module similarly registers a CEL backend, which encodes > CELs as JSON, and publishes them to an AMQP connection. > > > Diffs > - > > /branches/13/res/res_pjsip_config_wizard.c 430900 > /branches/13/res/res_amqp.exports.in PRE-CREATION > /branches/13/res/res_amqp.c PRE-CREATION > /branches/13/res/amqp/internal.h PRE-CREATION > /branches/13/res/amqp/config.c PRE-CREATION > /branches/13/res/amqp/cli.c PRE-CREATION > /branches/13/res/Makefile 430900 > /branches/13/makeopts.in 430900 > /branches/13/include/asterisk/cel.h 430900 > /branches/13/include/asterisk/autoconfig.h.in 430900 > /branches/13/include/asterisk/amqp.h PRE-CREATION > /branches/13/contrib/scripts/install_prereq 430900 > /branches/13/configure.ac 430900 > /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION > /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION > /branches/13/configs/samples/amqp.conf.sample PRE-CREATION > /branches/13/cel/cel_amqp.c PRE-CREATION > /branches/13/cdr/cdr_amqp.c PRE-CREATION > /branches/13/build_tools/menuselect-deps.in 430900 > > Diff: https://reviewboard.asterisk.org/r/4365/diff/ > > > Testing > --- > > Ran amqp-consume to print messages sent to queues. > > Confirmed that the amqp test command sent messages properly. > > Confirmed that CDRs were properly sent. > > Confirmed that CELs were properly sent. > > > Thanks, > > David Lee > > -- _ -- 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] 4327: Various fixes for OS X
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/ --- (Updated Jan. 26, 2015, 8:49 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 431092 Repository: Asterisk Description --- This patch addresses compilation errors on OS X. It's been a while, so there's quite a few things. * Fixed __attribute__ decls in route.h to be portable. * Fixed htonll and ntohll to work when they are defined as macros. * Replaced sem_t usage with our ast_sem wrapper. * Added ast_sem_timedwait to our ast_sem wrapper. * Fixed some GCC 4.9 warnings using sig*set() functions. * Fixed some format strings for portability. Diffs - /branches/13/res/res_timing_kqueue.c 431029 /branches/13/main/sem.c 431029 /branches/13/main/rtp_engine.c 431029 /branches/13/main/bridge_channel.c 431029 /branches/13/main/asterisk.c 431029 /branches/13/main/app.c 431029 /branches/13/include/asterisk/sem.h 431029 /branches/13/include/asterisk/autoconfig.h.in 431029 /branches/13/funcs/func_presencestate.c 431029 /branches/13/configure.ac 431029 /branches/13/configure UNKNOWN /branches/13/channels/sip/include/route.h 431029 Diff: https://reviewboard.asterisk.org/r/4327/diff/ Testing --- Compiled on both OS X and Linux. Thanks, David Lee -- _ -- 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] 4327: Various fixes for OS X
> On Jan. 23, 2015, 9:54 a.m., Matt Jordan wrote: > > /branches/13/main/rtp_engine.c, lines 1949-1952 > > <https://reviewboard.asterisk.org/r/4327/diff/3/?file=70526#file70526line1949> > > > > You got overcome by events here: these should be cast to unsigned long. > > > > It may no longer trip a compiler warning, however. It's a weird __darwin_suseconds_t on OS X, which is why the cast is needed. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/#review14270 ------- On Jan. 23, 2015, 12:02 p.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4327/ > --- > > (Updated Jan. 23, 2015, 12:02 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > --- > > This patch addresses compilation errors on OS X. It's been a while, so > there's quite a few things. > > * Fixed __attribute__ decls in route.h to be portable. > * Fixed htonll and ntohll to work when they are defined as macros. > * Replaced sem_t usage with our ast_sem wrapper. > * Added ast_sem_timedwait to our ast_sem wrapper. > * Fixed some GCC 4.9 warnings using sig*set() functions. > * Fixed some format strings for portability. > > > Diffs > - > > /branches/13/res/res_timing_kqueue.c 431029 > /branches/13/main/sem.c 431029 > /branches/13/main/rtp_engine.c 431029 > /branches/13/main/bridge_channel.c 431029 > /branches/13/main/asterisk.c 431029 > /branches/13/main/app.c 431029 > /branches/13/include/asterisk/sem.h 431029 > /branches/13/include/asterisk/autoconfig.h.in 431029 > /branches/13/funcs/func_presencestate.c 431029 > /branches/13/configure.ac 431029 > /branches/13/configure UNKNOWN > /branches/13/channels/sip/include/route.h 431029 > > Diff: https://reviewboard.asterisk.org/r/4327/diff/ > > > Testing > --- > > Compiled on both OS X and Linux. > > > Thanks, > > David Lee > > -- _ -- 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] 4327: Various fixes for OS X
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/ --- (Updated Jan. 23, 2015, 12:02 p.m.) Review request for Asterisk Developers. Changes --- * Updated diff to latest 13 branch * Fixed the cast for a tv_usec. * Committed the pjproject dependency fix. Repository: Asterisk Description (updated) --- This patch addresses compilation errors on OS X. It's been a while, so there's quite a few things. * Fixed __attribute__ decls in route.h to be portable. * Fixed htonll and ntohll to work when they are defined as macros. * Replaced sem_t usage with our ast_sem wrapper. * Added ast_sem_timedwait to our ast_sem wrapper. * Fixed some GCC 4.9 warnings using sig*set() functions. * Fixed some format strings for portability. Diffs (updated) - /branches/13/res/res_timing_kqueue.c 431029 /branches/13/main/sem.c 431029 /branches/13/main/rtp_engine.c 431029 /branches/13/main/bridge_channel.c 431029 /branches/13/main/asterisk.c 431029 /branches/13/main/app.c 431029 /branches/13/include/asterisk/sem.h 431029 /branches/13/include/asterisk/autoconfig.h.in 431029 /branches/13/funcs/func_presencestate.c 431029 /branches/13/configure.ac 431029 /branches/13/configure UNKNOWN /branches/13/channels/sip/include/route.h 431029 Diff: https://reviewboard.asterisk.org/r/4327/diff/ Testing --- Compiled on both OS X and Linux. Thanks, David Lee -- _ -- 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] 4365: Adding AMQP backend for CDR and CEL
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/ --- (Updated Jan. 23, 2015, 10:37 a.m.) Review request for Asterisk Developers. Changes --- Review feedback. * Removed some bogus doc comments * Fixed spacing in .conf files * Added some error logging Repository: Asterisk Description --- This patch adds an AMQP backend for both CDR and CEL. This allows these logs to be dispatched to a message broker, such as RabbitMQ, where they can reliably be queued for transmission to the handling application. The res_amqp module adds core AMQP protocol support. The provided API only supports publishing, since that's all that's needed for CDR/CEL. The AMQP feature of supporting multiple 'channels' per connection was also omitted. The underlying librabbitmq library is not thread safe, so the multiplexing benefits are limited. It also makes using the library more complicated, since clients have to worry about both connections and channels. The cdr_amqp module simply registers a CDR backend which encodes CDRs as JSON, and publishes them to an AMQP connection. The cel_amqp module similarly registers a CEL backend, which encodes CELs as JSON, and publishes them to an AMQP connection. Diffs (updated) - /branches/13/res/res_pjsip_config_wizard.c 430900 /branches/13/res/res_amqp.exports.in PRE-CREATION /branches/13/res/res_amqp.c PRE-CREATION /branches/13/res/amqp/internal.h PRE-CREATION /branches/13/res/amqp/config.c PRE-CREATION /branches/13/res/amqp/cli.c PRE-CREATION /branches/13/res/Makefile 430900 /branches/13/makeopts.in 430900 /branches/13/include/asterisk/cel.h 430900 /branches/13/include/asterisk/autoconfig.h.in 430900 /branches/13/include/asterisk/amqp.h PRE-CREATION /branches/13/contrib/scripts/install_prereq 430900 /branches/13/configure.ac 430900 /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/amqp.conf.sample PRE-CREATION /branches/13/cel/cel_amqp.c PRE-CREATION /branches/13/cdr/cdr_amqp.c PRE-CREATION /branches/13/build_tools/menuselect-deps.in 430900 Diff: https://reviewboard.asterisk.org/r/4365/diff/ Testing --- Ran amqp-consume to print messages sent to queues. Confirmed that the amqp test command sent messages properly. Confirmed that CDRs were properly sent. Confirmed that CELs were properly sent. Thanks, David Lee -- _ -- 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] 4365: Adding AMQP backend for CDR and CEL
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4365/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- This patch adds an AMQP backend for both CDR and CEL. This allows these logs to be dispatched to a message broker, such as RabbitMQ, where they can reliably be queued for transmission to the handling application. The res_amqp module adds core AMQP protocol support. The provided API only supports publishing, since that's all that's needed for CDR/CEL. The AMQP feature of supporting multiple 'channels' per connection was also omitted. The underlying librabbitmq library is not thread safe, so the multiplexing benefits are limited. It also makes using the library more complicated, since clients have to worry about both connections and channels. The cdr_amqp module simply registers a CDR backend which encodes CDRs as JSON, and publishes them to an AMQP connection. The cel_amqp module similarly registers a CEL backend, which encodes CELs as JSON, and publishes them to an AMQP connection. Diffs - /branches/13/res/res_pjsip_config_wizard.c 430900 /branches/13/res/res_amqp.exports.in PRE-CREATION /branches/13/res/res_amqp.c PRE-CREATION /branches/13/res/amqp/internal.h PRE-CREATION /branches/13/res/amqp/config.c PRE-CREATION /branches/13/res/amqp/cli.c PRE-CREATION /branches/13/res/Makefile 430900 /branches/13/makeopts.in 430900 /branches/13/include/asterisk/cel.h 430900 /branches/13/include/asterisk/autoconfig.h.in 430900 /branches/13/include/asterisk/amqp.h PRE-CREATION /branches/13/contrib/scripts/install_prereq 430900 /branches/13/configure.ac 430900 /branches/13/configs/samples/cel_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/cdr_amqp.conf.sample PRE-CREATION /branches/13/configs/samples/amqp.conf.sample PRE-CREATION /branches/13/cel/cel_amqp.c PRE-CREATION /branches/13/cdr/cdr_amqp.c PRE-CREATION /branches/13/build_tools/menuselect-deps.in 430900 Diff: https://reviewboard.asterisk.org/r/4365/diff/ Testing --- Ran amqp-consume to print messages sent to queues. Confirmed that the amqp test command sent messages properly. Confirmed that CDRs were properly sent. Confirmed that CELs were properly sent. Thanks, David Lee -- _ -- 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] 4327: Various fixes for OS X
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/ --- (Updated Jan. 13, 2015, 8:14 p.m.) Review request for Asterisk Developers. Changes --- Fix testing for /sbin/launchd from menuselect Repository: Asterisk Description --- This patch addresses compilation errors on OS X. It's been a while, so there's quite a few things. * Fixed __attribute__ decls in route.h to be portable. * Fixed htonll and ntohll to work when they are defined as macros. * Replaced sem_t usage with our ast_sem wrapper. * Added ast_sem_timedwait to our ast_sem wrapper. * Fixed some GCC 4.9 warnings using sig*set() functions. * Fixed some format strings for portability. * Added pjproject dependency to res_pjsip_config_wizard.c. Diffs (updated) - /branches/13/res/res_timing_kqueue.c 430428 /branches/13/res/res_pjsip_config_wizard.c 430428 /branches/13/main/sem.c 430428 /branches/13/main/rtp_engine.c 430428 /branches/13/main/bridge_channel.c 430428 /branches/13/main/asterisk.c 430428 /branches/13/main/app.c 430428 /branches/13/include/asterisk/sem.h 430428 /branches/13/include/asterisk/autoconfig.h.in 430428 /branches/13/funcs/func_presencestate.c 430428 /branches/13/configure.ac 430428 /branches/13/configure UNKNOWN /branches/13/channels/sip/include/route.h 430428 Diff: https://reviewboard.asterisk.org/r/4327/diff/ Testing --- Compiled on both OS X and Linux. Thanks, David Lee -- _ -- 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] 4327: Various fixes for OS X
> On Jan. 9, 2015, 11:16 a.m., George Joseph wrote: > > There are a few recent issues that I think this addresses... > > ASTERISK-24539 > > ASTERISK-24544 > > > > Does this patch also address > > ASTERISK-24559 > > ASTERISK-24565 > > ? > > > > > > David Lee wrote: > > ASTERISK-24539 > > ASTERISK-24544 > > Yes. > > > ASTERISK-24559 > > Probably not. > > > ASTERISK-24565 > > No. > > George Joseph wrote: > Ok, can you add the 2 yeses to the review? For ASTERISK-24559 (app_voicemail notify_new_message() forked process crashes on OS X), that beyond my current scope. For ASTERISK-24565 (kqueue not working correctly on OSX), I did fix the compile errors, but it still doesn't function properly. I spent a few minutes looking at it, but it looks pretty fundamentally broken to me. (As in the kqueue code there doesn't line up with kqueue timer examples that I see.) I wouldn't know how to get it to work without learning a lot more about kqueue than I care to learn. FWIW, it appears in r271657, there was an attempt to disable kqueue on OS X anyways, but the statement that was introduced doesn't seem to be doing that anyways. I may try to fix that, so in the very least no one tries to use it on OS X. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/#review14151 --- On Jan. 9, 2015, 5:31 p.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4327/ > --- > > (Updated Jan. 9, 2015, 5:31 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > --- > > This patch addresses compilation errors on OS X. It's been a while, so > there's quite a few things. > > * Fixed __attribute__ decls in route.h to be portable. > * Fixed htonll and ntohll to work when they are defined as macros. > * Replaced sem_t usage with our ast_sem wrapper. > * Added ast_sem_timedwait to our ast_sem wrapper. > * Fixed some GCC 4.9 warnings using sig*set() functions. > * Fixed some format strings for portability. > * Added pjproject dependency to res_pjsip_config_wizard.c. > > > Diffs > - > > /branches/13/res/res_timing_kqueue.c 430428 > /branches/13/res/res_pjsip_config_wizard.c 430428 > /branches/13/main/sem.c 430428 > /branches/13/main/rtp_engine.c 430428 > /branches/13/main/bridge_channel.c 430428 > /branches/13/main/asterisk.c 430428 > /branches/13/main/app.c 430428 > /branches/13/include/asterisk/sem.h 430428 > /branches/13/include/asterisk/autoconfig.h.in 430428 > /branches/13/funcs/func_presencestate.c 430428 > /branches/13/configure.ac 430428 > /branches/13/configure UNKNOWN > /branches/13/channels/sip/include/route.h 430428 > > Diff: https://reviewboard.asterisk.org/r/4327/diff/ > > > Testing > --- > > Compiled on both OS X and Linux. > > > Thanks, > > David Lee > > -- _ -- 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] 4327: Various fixes for OS X
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/ --- (Updated Jan. 9, 2015, 5:31 p.m.) Review request for Asterisk Developers. Changes --- Fixed compile errors for res_timing_kqueue.c Repository: Asterisk Description --- This patch addresses compilation errors on OS X. It's been a while, so there's quite a few things. * Fixed __attribute__ decls in route.h to be portable. * Fixed htonll and ntohll to work when they are defined as macros. * Replaced sem_t usage with our ast_sem wrapper. * Added ast_sem_timedwait to our ast_sem wrapper. * Fixed some GCC 4.9 warnings using sig*set() functions. * Fixed some format strings for portability. * Added pjproject dependency to res_pjsip_config_wizard.c. Diffs (updated) - /branches/13/res/res_timing_kqueue.c 430428 /branches/13/res/res_pjsip_config_wizard.c 430428 /branches/13/main/sem.c 430428 /branches/13/main/rtp_engine.c 430428 /branches/13/main/bridge_channel.c 430428 /branches/13/main/asterisk.c 430428 /branches/13/main/app.c 430428 /branches/13/include/asterisk/sem.h 430428 /branches/13/include/asterisk/autoconfig.h.in 430428 /branches/13/funcs/func_presencestate.c 430428 /branches/13/configure.ac 430428 /branches/13/configure UNKNOWN /branches/13/channels/sip/include/route.h 430428 Diff: https://reviewboard.asterisk.org/r/4327/diff/ Testing --- Compiled on both OS X and Linux. Thanks, David Lee -- _ -- 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] 4327: Various fixes for OS X
> On Jan. 9, 2015, 11:16 a.m., George Joseph wrote: > > There are a few recent issues that I think this addresses... > > ASTERISK-24539 > > ASTERISK-24544 > > > > Does this patch also address > > ASTERISK-24559 > > ASTERISK-24565 > > ? > > > > > ASTERISK-24539 > ASTERISK-24544 Yes. > ASTERISK-24559 Probably not. > ASTERISK-24565 No. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/#review14151 ------- On Jan. 9, 2015, 11:05 a.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4327/ > --- > > (Updated Jan. 9, 2015, 11:05 a.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > --- > > This patch addresses compilation errors on OS X. It's been a while, so > there's quite a few things. > > * Fixed __attribute__ decls in route.h to be portable. > * Fixed htonll and ntohll to work when they are defined as macros. > * Replaced sem_t usage with our ast_sem wrapper. > * Added ast_sem_timedwait to our ast_sem wrapper. > * Fixed some GCC 4.9 warnings using sig*set() functions. > * Fixed some format strings for portability. > * Added pjproject dependency to res_pjsip_config_wizard.c. > > > Diffs > - > > /branches/13/res/res_pjsip_config_wizard.c 430428 > /branches/13/main/sem.c 430428 > /branches/13/main/rtp_engine.c 430428 > /branches/13/main/bridge_channel.c 430428 > /branches/13/main/asterisk.c 430428 > /branches/13/main/app.c 430428 > /branches/13/include/asterisk/sem.h 430428 > /branches/13/include/asterisk/autoconfig.h.in 430428 > /branches/13/funcs/func_presencestate.c 430428 > /branches/13/configure.ac 430428 > /branches/13/configure UNKNOWN > /branches/13/channels/sip/include/route.h 430428 > > Diff: https://reviewboard.asterisk.org/r/4327/diff/ > > > Testing > --- > > Compiled on both OS X and Linux. > > > Thanks, > > David Lee > > -- _ -- 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] 4327: Various fixes for OS X
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4327/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- This patch addresses compilation errors on OS X. It's been a while, so there's quite a few things. * Fixed __attribute__ decls in route.h to be portable. * Fixed htonll and ntohll to work when they are defined as macros. * Replaced sem_t usage with our ast_sem wrapper. * Added ast_sem_timedwait to our ast_sem wrapper. * Fixed some GCC 4.9 warnings using sig*set() functions. * Fixed some format strings for portability. * Added pjproject dependency to res_pjsip_config_wizard.c. Diffs - /branches/13/res/res_pjsip_config_wizard.c 430428 /branches/13/main/sem.c 430428 /branches/13/main/rtp_engine.c 430428 /branches/13/main/bridge_channel.c 430428 /branches/13/main/asterisk.c 430428 /branches/13/main/app.c 430428 /branches/13/include/asterisk/sem.h 430428 /branches/13/include/asterisk/autoconfig.h.in 430428 /branches/13/funcs/func_presencestate.c 430428 /branches/13/configure.ac 430428 /branches/13/configure UNKNOWN /branches/13/channels/sip/include/route.h 430428 Diff: https://reviewboard.asterisk.org/r/4327/diff/ Testing --- Compiled on both OS X and Linux. Thanks, David Lee -- _ -- 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] 2826: Debug threads: avoid double-initialization of lock tracking
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2826/ --- (Updated Dec. 12, 2014, 5:31 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Matt Jordan. Changes --- Committed in revision 429539 Bugs: ASTERISK-19463 https://issues.asterisk.org/jira/browse/ASTERISK-19463 Repository: Asterisk Description --- This patch corrects a consistency issue with debug threads that I noticed while fixing ASTERISK-22455. The initialization of a mutex's lock tracking structure was not protected in a critical section. This is fine for any mutex that is explicitly initialized, but a static mutex may have its lock tracking double-initialized if multiple threads attempt the first lock simultaneously. This patch creates a global mutex to properly serialize the initialization of the lock tracking structure for a mutex. It also changes lock.c to properly handle allocation failures of the lock tracking structure. Diffs - /branches/1.8/main/lock.c 398421 /branches/1.8/include/asterisk/lock.h 398421 Diff: https://reviewboard.asterisk.org/r/2826/diff/ Testing --- I propose we setup Bamboo to run the TestSuite with DEBUG_THREADS enabled on this branch nightly for a few weeks. Thanks, David Lee -- _ -- 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] 4258: Fix crash for sorcery misconfigs
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4258/ --- (Updated Dec. 12, 2014, 9 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 429457 Repository: Asterisk Description --- res_pjsip_outbound_publish was missing the CHECK_PJSIP_MODULE_LOADED() call in load_module, and would crash with a segfault if res_pjsip declined to load. Diffs - /branches/13/res/res_pjsip_outbound_publish.c 429386 Diff: https://reviewboard.asterisk.org/r/4258/diff/ Testing --- Added res_pjsip realtime config to sorcery.conf (see https://wiki.asterisk.org/wiki/x/wgaUAQ) without configuring extconfig.conf. Asterisk crashes on startup. Applied patch, build, install, start Asterisk, no crash. Thanks, David Lee -- _ -- 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] 4258: Fix crash for sorcery misconfigs
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4258/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- res_pjsip_outbound_publish was missing the CHECK_PJSIP_MODULE_LOADED() call in load_module, and would crash with a segfault if res_pjsip declined to load. Diffs - /branches/13/res/res_pjsip_outbound_publish.c 429386 Diff: https://reviewboard.asterisk.org/r/4258/diff/ Testing --- Added res_pjsip realtime config to sorcery.conf (see https://wiki.asterisk.org/wiki/x/wgaUAQ) without configuring extconfig.conf. Asterisk crashes on startup. Applied patch, build, install, start Asterisk, no crash. Thanks, David Lee -- _ -- 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] 3972: Only install dahdi_span_config_hook if DAHDI is enabled
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3972/ --- (Updated Sept. 18, 2014, 10 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 423281 Repository: Asterisk Description --- I'm a bit weird, and I configure asterisk with --prefix=/opt/asterisk, so that I can install it without running as root. The install script for the DAHDI hook scripts were hard coded to install into /usr/share, which foils my weirdness. This patch changes the install to only install the hook script if DAHDI is enabled. It also adds the script to the uninstall task, and moves the DAHDI_UDEV_HOOK_DIR variable so that it's not between the _MAKEOPTS variables and their comment. Diffs - /branches/13/makeopts.in 422582 /branches/13/Makefile 422582 Diff: https://reviewboard.asterisk.org/r/3972/diff/ Testing --- Ran ./configure && make all install both with and without DAHDI, confirming that it worked as expected. Thanks, David Lee -- _ -- 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] 3972: Only install dahdi_span_config_hook if DAHDI is enabled
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3972/ --- (Updated Sept. 4, 2014, 1:35 p.m.) Review request for Asterisk Developers. Summary (updated) - Only install dahdi_span_config_hook if DAHDI is enabled Repository: Asterisk Description --- I'm a bit weird, and I configure asterisk with --prefix=/opt/asterisk, so that I can install it without running as root. The install script for the DAHDI hook scripts were hard coded to install into /usr/share, which foils my weirdness. This patch changes the install to only install the hook script if DAHDI is enabled. It also adds the script to the uninstall task, and moves the DAHDI_UDEV_HOOK_DIR variable so that it's not between the _MAKEOPTS variables and their comment. Diffs - /branches/13/makeopts.in 422582 /branches/13/Makefile 422582 Diff: https://reviewboard.asterisk.org/r/3972/diff/ Testing --- Ran ./configure && make all install both with and without DAHDI, confirming that it worked as expected. Thanks, David Lee -- _ -- 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] 3972: Change DAHDI_UDEV_HOOK_DIR to honor --prefix
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3972/ --- (Updated Sept. 4, 2014, 1:33 p.m.) Review request for Asterisk Developers. Changes --- Updated patch to only install hook script if DAHDI is enabled. Repository: Asterisk Description (updated) --- I'm a bit weird, and I configure asterisk with --prefix=/opt/asterisk, so that I can install it without running as root. The install script for the DAHDI hook scripts were hard coded to install into /usr/share, which foils my weirdness. This patch changes the install to only install the hook script if DAHDI is enabled. It also adds the script to the uninstall task, and moves the DAHDI_UDEV_HOOK_DIR variable so that it's not between the _MAKEOPTS variables and their comment. Diffs (updated) - /branches/13/makeopts.in 422582 /branches/13/Makefile 422582 Diff: https://reviewboard.asterisk.org/r/3972/diff/ Testing (updated) --- Ran ./configure && make all install both with and without DAHDI, confirming that it worked as expected. Thanks, David Lee -- _ -- 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] 3972: Change DAHDI_UDEV_HOOK_DIR to honor --prefix
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3972/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- I'm a bit wierd, and I configure asterisk with --prefix=/opt/asterisk, so that I can install it without running as root. The install script for the DAHDI hook scripts were hard coded to install into /usr/share, which foils my weirdness. This patch changes the install location for the DAHDI hook scripts to honor --prefix. It also moves the variable in the Makefile so that it doesn't get between the _MAKEOPTS variables and the comment describing them. Diffs - /branches/13/Makefile 422556 Diff: https://reviewboard.asterisk.org/r/3972/diff/ Testing --- Ran ./configure && make all install to verify that the hook script installed into /usr/share/dahdi/span_config.d. Ran ./configure --prefix=/opt/asterisk && make all install to verify that the hook script installed into /opt/Asterisk/share/dahdi/span_config.d Thanks, David Lee -- _ -- 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] 3427: ARI: Add tones playback resource
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3427/#review11529 --- /trunk/CHANGES <https://reviewboard.asterisk.org/r/3427/#comment21260> How hard would it be to add something like ?timeMillis= to the URI for tones? Or are tones usually something like busy, where you usually just want to play the tone until the call ends? /trunk/main/app.c <https://reviewboard.asterisk.org/r/3427/#comment21261> Rather than just have this as a useless wrapper function, you should probably just implement ast_control_tones here directly. - David Lee On April 8, 2014, 5:46 p.m., Jonathan Rose wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3427/ > --- > > (Updated April 8, 2014, 5:46 p.m.) > > > Review request for Asterisk Developers, David Lee, Joshua Colp, and Matt > Jordan. > > > Bugs: ASTERISK-23433 > https://issues.asterisk.org/jira/browse/ASTERISK-23433 > > > Repository: Asterisk > > > Description > --- > > Adds a tones URI type to the playback resource. The tone can be specified by > name (from indications.conf) or by a tone pattern (comma separate > pitch/duration list). Tones aren't like regular sounds in that they must be > canceled manually before the control can move on to the next item in the > queue. > > Tones are capable of being paused and resumed (although they will always > resumed from the beginning of the tone), restarted, and stopped. Tones are > not capable of being fastforwarded, skipped into by a duration, or rewound by > a small amount. Those operations unfortunately report success rather than a > lack of availability right now due to how control on playbacks is defined (a > playback is either completely controllable or not). I could probably add a > little more granularity to that if we want it. > > > Diffs > - > > /trunk/res/res_stasis_playback.c 411884 > /trunk/main/app.c 411884 > /trunk/include/asterisk/app.h 411884 > /trunk/CHANGES 411884 > > Diff: https://reviewboard.asterisk.org/r/3427/diff/ > > > Testing > --- > > I've written two testsuite tests (one for channels, one for bridges) which > queue and stop tones with playback. I'll be posting them before too long. > I've also performed all the basic control operations by hand. > > > Thanks, > > Jonathan Rose > > -- _ -- 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] 3336: ARI: Ensure ChannelEnteredBridge messages get to the managing application
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3336/#review11152 --- Ship it! I think it just needs a comment; otherwise it looks good! branches/12/res/stasis/control.c <https://reviewboard.asterisk.org/r/3336/#comment20794> This probably deserves a comment as to why we're doing a subscribe here, but we also have other logic for subscribing to the bridge a channel is in. Actually, if this works, then we might be able to get rid of the other logic. This method is certainly a lot simpler. - David Lee On March 12, 2014, 8:33 a.m., opticron wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3336/ > --- > > (Updated March 12, 2014, 8:33 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23295 > https://issues.asterisk.org/jira/browse/ASTERISK-23295 > > > Repository: Asterisk > > > Description > --- > > This fixes an issue where a Stasis application running over ARI and > subscribed to ari/events could miss the ChannelEnteredBridge event because it > did not subscribe to the new bridge fast enough. > > To accomplish this, it subscribes the application controlling the channel to > the new bridge before adding it to that bridge which required the > stasis_app_control structure to maintain a reference to the stasis_app. > > > Diffs > - > > branches/12/res/stasis/control.c 410448 > branches/12/res/stasis/control.h 410448 > branches/12/res/res_stasis.c 410448 > > Diff: https://reviewboard.asterisk.org/r/3336/diff/ > > > Testing > --- > > Ensured that the ARI tests still functioned since I was unable to reproduce > the bug this fixes. > > > Thanks, > > opticron > > -- _ -- 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] 3273: Corrected cross-platform stat nanosecond code.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3273/ --- (Updated March 5, 2014, 11:19 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Repository: Asterisk Description --- When nanosecond time resolution was added for identifying config file changes, it didn't cover all of the myriad of ways that one might obtain nanosecond time resolution off of struct stat. Rather than complicate the #if even further figuring out one system from the next, this patch directly tests for the three struct members I know about today, and #ifdef's accordingly. Diffs - /branches/1.8/main/config.c 409079 /branches/1.8/include/asterisk/autoconfig.h.in 409079 /branches/1.8/configure.ac 409079 /branches/1.8/configure UNKNOWN Diff: https://reviewboard.asterisk.org/r/3273/diff/ Testing --- Compiled on Ubuntu 13.10 and OS X 10.9.2. Thanks, David Lee -- _ -- 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] 3273: Corrected cross-platform stat nanosecond code.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3273/ --- (Updated Feb. 27, 2014, 11:33 a.m.) Review request for Asterisk Developers. Changes --- Moved the warning re: not finding a nanoseconds field from config.c to the configure script. Repository: Asterisk Description --- When nanosecond time resolution was added for identifying config file changes, it didn't cover all of the myriad of ways that one might obtain nanosecond time resolution off of struct stat. Rather than complicate the #if even further figuring out one system from the next, this patch directly tests for the three struct members I know about today, and #ifdef's accordingly. Diffs (updated) - /branches/1.8/main/config.c 409079 /branches/1.8/include/asterisk/autoconfig.h.in 409079 /branches/1.8/configure.ac 409079 /branches/1.8/configure UNKNOWN Diff: https://reviewboard.asterisk.org/r/3273/diff/ Testing --- Compiled on Ubuntu 13.10 and OS X 10.9.2. Thanks, David Lee -- _ -- 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] 3273: Corrected cross-platform stat nanosecond code.
> On Feb. 27, 2014, 10:48 a.m., rmudgett wrote: > > /branches/1.8/main/config.c, line 1102 > > <https://reviewboard.asterisk.org/r/3273/diff/1/?file=54883#file54883line1102> > > > > It is acceptable to just set the nanoseconds to zero if the platform > > doesn't support the resolution. However, the warning should not be done > > here. That warning will cause dev mode to always fail. The warning should > > be in the configure log file if it is done anywhere. I was surprised enough at finding three ways of doing the same thing, I wanted development builds to blow up if we stumbled upon a fourth. Probably not the best place to do it here, and there probably are valid systems that don't support nanosecond times at all. Moving the warning to the configure script. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3273/#review10973 ----------- On Feb. 27, 2014, 10:37 a.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3273/ > --- > > (Updated Feb. 27, 2014, 10:37 a.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > --- > > When nanosecond time resolution was added for identifying config file > changes, it didn't cover all of the myriad of ways that one might obtain > nanosecond time resolution off of struct stat. > > Rather than complicate the #if even further figuring out one system from > the next, this patch directly tests for the three struct members I know > about today, and #ifdef's accordingly. > > > Diffs > - > > /branches/1.8/main/config.c 409079 > /branches/1.8/include/asterisk/autoconfig.h.in 409079 > /branches/1.8/configure.ac 409079 > /branches/1.8/configure UNKNOWN > > Diff: https://reviewboard.asterisk.org/r/3273/diff/ > > > Testing > --- > > Compiled on Ubuntu 13.10 and OS X 10.9.2. > > > Thanks, > > David Lee > > -- _ -- 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] 3273: Corrected cross-platform stat nanosecond code.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3273/ --- (Updated Feb. 27, 2014, 10:37 a.m.) Review request for Asterisk Developers. Summary (updated) - Corrected cross-platform stat nanosecond code. Repository: Asterisk Description --- When nanosecond time resolution was added for identifying config file changes, it didn't cover all of the myriad of ways that one might obtain nanosecond time resolution off of struct stat. Rather than complicate the #if even further figuring out one system from the next, this patch directly tests for the three struct members I know about today, and #ifdef's accordingly. Diffs - /branches/1.8/main/config.c 409079 /branches/1.8/include/asterisk/autoconfig.h.in 409079 /branches/1.8/configure.ac 409079 /branches/1.8/configure UNKNOWN Diff: https://reviewboard.asterisk.org/r/3273/diff/ Testing --- Compiled on Ubuntu 13.10 and OS X 10.9.2. Thanks, David Lee -- _ -- 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] 3273: Correct cross-platform stat nanosecond code.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3273/ --- Review request for Asterisk Developers. Repository: Asterisk Description --- When nanosecond time resolution was added for identifying config file changes, it didn't cover all of the myriad of ways that one might obtain nanosecond time resolution off of struct stat. Rather than complicate the #if even further figuring out one system from the next, this patch directly tests for the three struct members I know about today, and #ifdef's accordingly. Diffs - /branches/1.8/main/config.c 409079 /branches/1.8/include/asterisk/autoconfig.h.in 409079 /branches/1.8/configure.ac 409079 /branches/1.8/configure UNKNOWN Diff: https://reviewboard.asterisk.org/r/3273/diff/ Testing --- Compiled on Ubuntu 13.10 and OS X 10.9.2. Thanks, David Lee -- _ -- 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] 3099: CDRs and Stasis take 2: Synchronize publication of application CDR messages to the CDR engine subscription
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3099/#review10564 --- Ship it! I've just looked at the Stasis code, and have no more objections. - David Lee On Jan. 9, 2014, 5:34 p.m., Matt Jordan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3099/ > --- > > (Updated Jan. 9, 2014, 5:34 p.m.) > > > Review request for Asterisk Developers and David Lee. > > > Bugs: ASTERISK-22884 > https://issues.asterisk.org/jira/browse/ASTERISK-22884 > > > Repository: Asterisk > > > Description > --- > > In https://reviewboard.asterisk.org/r/3057/, applications and functions that > manipulate CDRs were made to interact over Stasis. This was done to > synchronize manipulations of CDRs from the dialplan with the updates the > engine itself receives over the message bus. > > This change rested on a faulty premise: that messages published to the CDR > topic or to a topic that forwards to the CDR topic are synchronized with the > messages handled by the CDR topic subscription in the CDR engine. This is not > the case. There is no ordering guaranteed for two messages published to the > same topic; ordering is only guaranteed if a message is published to the same > subscriber. > > Note that the patch actually still fixed a bunch of test failures by virtue > of publishing the messages in the first place to the channel topic; however, > the hangup handler tests - which use the CDR function to read a value from > the engine - did still fail after this patch. Reading data requires an > explicit synchronization with the engine. > > In order to fix this problem, this patch does the following: > (1) It provides a new way to publish a message. It is now possible to publish > a message and specify which stasis_subscription you want to synchronize on. A > message published in such a fashion is still published to all subscribers of > the topic (which is needed; you don't know who all wants your message), but > the call will block until the specified subscriber handles the message. > (2) It updates the CDR engine to exposed its message router (which contains > the subscription). CDR related applications now synchronize on the message > router when publishing information. As a result, the CDR topic/router now > persist across CDR engine disablings; the only thing that is removed when the > engine is disabled are the forwarding subscriptions. This keeps a lot of > quirky lifetime issues from becoming serious problems, while still > maintaining performance if the CDR engine is disabled. > > > Diffs > - > > /branches/12/tests/test_stasis.c 405210 > /branches/12/main/stasis_message_router.c 405210 > /branches/12/main/stasis.c 405210 > /branches/12/main/cdr.c 405210 > /branches/12/include/asterisk/stasis_message_router.h 405210 > /branches/12/include/asterisk/stasis.h 405210 > /branches/12/include/asterisk/cdr.h 405210 > /branches/12/funcs/func_cdr.c 405210 > /branches/12/apps/app_forkcdr.c 405210 > /branches/12/apps/app_cdr.c 405210 > > Diff: https://reviewboard.asterisk.org/r/3099/diff/ > > > Testing > --- > > Unit test for publishing synchronously was added. It passes. > > All stasis and CDR unit tests pass. > > All CDR and hangup handler test in the test suite now pass. Previously, the > hangup handler tests would fail due to getting wrong data back from the CDR > engine when reading a value using the CDR function. > > > Thanks, > > Matt Jordan > > -- _ -- 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] 3099: CDRs and Stasis take 2: Synchronize publication of application CDR messages to the CDR engine subscription
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3099/#review10549 --- /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3099/#comment20001> I'm curious why you didn't just dispatch_message(sub, message, sync_sub == sub); As it's written, sync_sub will receive the message, even if it's not subscribed to the topic in question. But since we're just talking about the behavior of an error condition, I'm fine either way. - David Lee On Jan. 9, 2014, 9:16 a.m., Matt Jordan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3099/ > --- > > (Updated Jan. 9, 2014, 9:16 a.m.) > > > Review request for Asterisk Developers and David Lee. > > > Bugs: ASTERISK-22884 > https://issues.asterisk.org/jira/browse/ASTERISK-22884 > > > Repository: Asterisk > > > Description > --- > > In https://reviewboard.asterisk.org/r/3057/, applications and functions that > manipulate CDRs were made to interact over Stasis. This was done to > synchronize manipulations of CDRs from the dialplan with the updates the > engine itself receives over the message bus. > > This change rested on a faulty premise: that messages published to the CDR > topic or to a topic that forwards to the CDR topic are synchronized with the > messages handled by the CDR topic subscription in the CDR engine. This is not > the case. There is no ordering guaranteed for two messages published to the > same topic; ordering is only guaranteed if a message is published to the same > subscriber. > > Note that the patch actually still fixed a bunch of test failures by virtue > of publishing the messages in the first place to the channel topic; however, > the hangup handler tests - which use the CDR function to read a value from > the engine - did still fail after this patch. Reading data requires an > explicit synchronization with the engine. > > In order to fix this problem, this patch does the following: > (1) It provides a new way to publish a message. It is now possible to publish > a message and specify which stasis_subscription you want to synchronize on. A > message published in such a fashion is still published to all subscribers of > the topic (which is needed; you don't know who all wants your message), but > the call will block until the specified subscriber handles the message. > (2) It updates the CDR engine to exposed its message router (which contains > the subscription). CDR related applications now synchronize on the message > router when publishing information. As a result, the CDR topic/router now > persist across CDR engine disablings; the only thing that is removed when the > engine is disabled are the forwarding subscriptions. This keeps a lot of > quirky lifetime issues from becoming serious problems, while still > maintaining performance if the CDR engine is disabled. > > > Diffs > - > > /branches/12/tests/test_stasis.c 404591 > /branches/12/main/stasis_message_router.c 404591 > /branches/12/main/stasis.c 404591 > /branches/12/main/cdr.c 404591 > /branches/12/include/asterisk/stasis_message_router.h 404591 > /branches/12/include/asterisk/stasis.h 404591 > /branches/12/include/asterisk/cdr.h 404591 > /branches/12/funcs/func_cdr.c 404591 > /branches/12/apps/app_forkcdr.c 404591 > /branches/12/apps/app_cdr.c 404591 > > Diff: https://reviewboard.asterisk.org/r/3099/diff/ > > > Testing > --- > > Unit test for publishing synchronously was added. It passes. > > All stasis and CDR unit tests pass. > > All CDR and hangup handler test in the test suite now pass. Previously, the > hangup handler tests would fail due to getting wrong data back from the CDR > engine when reading a value using the CDR function. > > > Thanks, > > Matt Jordan > > -- _ -- 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] Asterisk Media Performance
On Jan 6, 2014, at 12:20 PM, Joshua Colp wrote: > David Lee (digium) wrote: >> >> Synchronizing the notifications with the format change may be tricky. If >> we’re not careful, we could end up in the situation where 1) the format >> changes but some bit of code gets the notification about the change too >> late or 2) we end up with extra locks/contention in the media path due >> to format change notifications. > > I think we can actually do it using a control frame. The stuff that cares > about the format would be in that path, and the control frame could just be > in front of the frame with the new format. As it passes through stuff can > change accordingly (change formats, smoother, etc). Works for me :-) -- David M. Lee Digium, Inc. | Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: www.digium.com & www.asterisk.org -- _ -- 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] Asterisk Media Performance
On Jan 6, 2014, at 11:54 AM, Joshua Colp wrote: > 1. Make ast_format an ao2 object > > I think what needs to happen is that ast_format becomes an immutable ao2 > reference counted object. Copying becomes bumping the reference count and > returning it. > > Additional related stuff can be attached and guaranteed that it will be > disposed of. This can include an actual list of attributes, and a pointer to > the format negotiatior. As a result operations become faster to do and memory > usage goes down. +1 for immutable objects. > 2. Audit format usage > > Times have changed and what we can do with Asterisk has also. We need to look > at how we are using formats internally and improve/optimize/change them. A > perfect example is the format one I used previously. Even though copying an > ast_format would become cheap there is no need to do it on every read frame. > Everything format related should be fast and quick. Agreed. In the media path, every little bit helps. > 3. Be less reactive > > We need to determine something has changed once (such as the format of > incoming media) and notify everything else involved. Reacting using the same > (or potentially more expensive) comparison logic at different points in the > chain is not needed. Synchronizing the notifications with the format change may be tricky. If we’re not careful, we could end up in the situation where 1) the format changes but some bit of code gets the notification about the change too late or 2) we end up with extra locks/contention in the media path due to format change notifications. Another option would be structuring ast_format so that comparisons are cheap. If it’s a refcounted immutable object, we might even be able to do it simply with a pointer comparison (or, at the very least, the compare function can be fast if comparing an ast_format with itself). Of course, I say that without actually looking at the code. Maybe there are other reasons to switch from reactive to notifications, or my concerns about extra locks and late notifications are just FUD. Just something to think about. -- David M. Lee Digium, Inc. | Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: www.digium.com & www.asterisk.org -- _ -- 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] [svn-commits] dlee: branch 12 r404006 - in /branches/12: configs/ main/
On Dec 23, 2013, at 6:44 PM, Paul Belanger wrote: > Please also update UPGRADE-12.txt and how we are breaking backwards > compatibility for upgrades. Done. Good catch! Thanks! -- David M. Lee Digium, Inc. | Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: www.digium.com & www.asterisk.org -- _ -- 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] 3092: http: Properly reject requests with Transfer-Encoding set
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3092/ --- (Updated Dec. 24, 2013, 10:41 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Bugs: ASTERISK-22486 https://issues.asterisk.org/jira/browse/ASTERISK-22486 Repository: Asterisk Description --- Asterisk does not support any of the transfer encodings specified in HTTP/1.1, other than the default "identity" encoding. According to RFC 2616: A server which receives an entity-body with a transfer-coding it does not understand SHOULD return 501 (Unimplemented), and close the connection. A server MUST NOT send transfer-codings to an HTTP/1.0 client. This patch adds the 501 Unimplemented response, instead of the hard work of actually implementing other recordings. This behavior is especially problematic for Node.js clients, which use chunked encoding by default. Diffs - /branches/12/main/http.c 404552 Diff: https://reviewboard.asterisk.org/r/3092/diff/ Testing --- HTTP requests using Node.js Thanks, David Lee -- _ -- 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] 3092: http: Properly reject requests with Transfer-Encoding set
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3092/ --- (Updated Dec. 24, 2013, 9:39 a.m.) Review request for Asterisk Developers. Changes --- Extract get_header function in http.c; fixed some copy/paste errors. Bugs: ASTERISK-22486 https://issues.asterisk.org/jira/browse/ASTERISK-22486 Repository: Asterisk Description --- Asterisk does not support any of the transfer encodings specified in HTTP/1.1, other than the default "identity" encoding. According to RFC 2616: A server which receives an entity-body with a transfer-coding it does not understand SHOULD return 501 (Unimplemented), and close the connection. A server MUST NOT send transfer-codings to an HTTP/1.0 client. This patch adds the 501 Unimplemented response, instead of the hard work of actually implementing other recordings. This behavior is especially problematic for Node.js clients, which use chunked encoding by default. Diffs (updated) - /branches/12/main/http.c 404552 Diff: https://reviewboard.asterisk.org/r/3092/diff/ Testing --- HTTP requests using Node.js Thanks, David Lee -- _ -- 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] 3092: http: Properly reject requests with Transfer-Encoding set
> On Dec. 24, 2013, 8:48 a.m., Joshua Colp wrote: > > /branches/12/main/http.c, lines 661-662 > > <https://reviewboard.asterisk.org/r/3092/diff/1/?file=49943#file49943line661> > > > > This is incorrect. /me blushes - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3092/#review10491 --- On Dec. 23, 2013, 5:09 p.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3092/ > --- > > (Updated Dec. 23, 2013, 5:09 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-22486 > https://issues.asterisk.org/jira/browse/ASTERISK-22486 > > > Repository: Asterisk > > > Description > --- > > Asterisk does not support any of the transfer encodings specified in > HTTP/1.1, other than the default "identity" encoding. > > According to RFC 2616: > >A server which receives an entity-body with a transfer-coding it does >not understand SHOULD return 501 (Unimplemented), and close the >connection. A server MUST NOT send transfer-codings to an HTTP/1.0 >client. > > This patch adds the 501 Unimplemented response, instead of the hard work > of actually implementing other recordings. > > This behavior is especially problematic for Node.js clients, which use > chunked encoding by default. > > > Diffs > - > > /branches/12/main/http.c 404552 > > Diff: https://reviewboard.asterisk.org/r/3092/diff/ > > > Testing > --- > > HTTP requests using Node.js > > > Thanks, > > David Lee > > -- _ -- 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] 3092: http: Properly reject requests with Transfer-Encoding set
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3092/ --- Review request for Asterisk Developers. Bugs: ASTERISK-22486 https://issues.asterisk.org/jira/browse/ASTERISK-22486 Repository: Asterisk Description --- Asterisk does not support any of the transfer encodings specified in HTTP/1.1, other than the default "identity" encoding. According to RFC 2616: A server which receives an entity-body with a transfer-coding it does not understand SHOULD return 501 (Unimplemented), and close the connection. A server MUST NOT send transfer-codings to an HTTP/1.0 client. This patch adds the 501 Unimplemented response, instead of the hard work of actually implementing other recordings. This behavior is especially problematic for Node.js clients, which use chunked encoding by default. Diffs - /branches/12/main/http.c 404552 Diff: https://reviewboard.asterisk.org/r/3092/diff/ Testing --- HTTP requests using Node.js Thanks, David Lee -- _ -- 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] 3088: ari: Remove support for specifying channel vars during origination.
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3088/
---
(Updated Dec. 20, 2013, 4:07 p.m.)
Status
--
This change has been marked as submitted.
Review request for Asterisk Developers and Matt Jordan.
Bugs: ASTERISK-23051
https://issues.asterisk.org/jira/browse/ASTERISK-23051
Repository: Asterisk
Description
---
When we added support for specifying channel variables for an
origination, we didn't consider how that would interact with another
feature, namely specifying request parameters in a JSON request body.
The method of specifying channel variables (as a flat JSON object passed
in the JSON body) interferes with parsing parameters out of the request
body.
Unfortunately, fixing this would be a backward incompatible change. In
the interest of keeping the API sane and keeping our release schedule,
we're dropping the feature for specifying channel variables in the
origination request.
We will bring the feature back soon, as a backward compatible addition
to the API.
Diffs
-
/branches/12/rest-api/api-docs/channels.json 404508
/branches/12/rest-api/api-docs/applications.json 404508
/branches/12/res/res_ari_channels.c 404508
/branches/12/res/ari/resource_channels.c 404508
/branches/12/res/ari/resource_channels.h 404508
Diff: https://reviewboard.asterisk.org/r/3088/diff/
Testing
---
Originated a call using a JSON post. Worked as expected.
curl -X POST -H "Content-Type: application/json" -d '{ "endpoint": "SIP/blink",
"app": "hello", "timeout": 30 }' -v
'http://localhost:8088/ari/channels?api_key=ari:ari'
Thanks,
David Lee
--
_
-- 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] 3088: ari: Remove support for specifying channel vars during origination.
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3088/
---
(Updated Dec. 20, 2013, 4 p.m.)
Review request for Asterisk Developers and Matt Jordan.
Changes
---
* Fixed doc
* Actually tested
Bugs: ASTERISK-23051
https://issues.asterisk.org/jira/browse/ASTERISK-23051
Repository: Asterisk
Description
---
When we added support for specifying channel variables for an
origination, we didn't consider how that would interact with another
feature, namely specifying request parameters in a JSON request body.
The method of specifying channel variables (as a flat JSON object passed
in the JSON body) interferes with parsing parameters out of the request
body.
Unfortunately, fixing this would be a backward incompatible change. In
the interest of keeping the API sane and keeping our release schedule,
we're dropping the feature for specifying channel variables in the
origination request.
We will bring the feature back soon, as a backward compatible addition
to the API.
Diffs (updated)
-
/branches/12/rest-api/api-docs/channels.json 404508
/branches/12/rest-api/api-docs/applications.json 404508
/branches/12/res/res_ari_channels.c 404508
/branches/12/res/ari/resource_channels.c 404508
/branches/12/res/ari/resource_channels.h 404508
Diff: https://reviewboard.asterisk.org/r/3088/diff/
Testing (updated)
---
Originated a call using a JSON post. Worked as expected.
curl -X POST -H "Content-Type: application/json" -d '{ "endpoint": "SIP/blink",
"app": "hello", "timeout": 30 }' -v
'http://localhost:8088/ari/channels?api_key=ari:ari'
Thanks,
David Lee
--
_
-- 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] 3088: ari: Remove support for specifying channel vars during origination.
> On Dec. 20, 2013, 3:51 p.m., Matt Jordan wrote: > > /branches/12/res/ari/resource_applications.h, line 91 > > <https://reviewboard.asterisk.org/r/3088/diff/1/?file=49930#file49930line91> > > > > Undo this - the comment is now correct in the code (deviceState) Actually, that's in the generated code. I fixed the doc in rest-api/api-docs/applications.json. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3088/#review10486 --- On Dec. 20, 2013, 3:50 p.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3088/ > --- > > (Updated Dec. 20, 2013, 3:50 p.m.) > > > Review request for Asterisk Developers and Matt Jordan. > > > Bugs: ASTERISK-23051 > https://issues.asterisk.org/jira/browse/ASTERISK-23051 > > > Repository: Asterisk > > > Description > --- > > When we added support for specifying channel variables for an > origination, we didn't consider how that would interact with another > feature, namely specifying request parameters in a JSON request body. > > The method of specifying channel variables (as a flat JSON object passed > in the JSON body) interferes with parsing parameters out of the request > body. > > Unfortunately, fixing this would be a backward incompatible change. In > the interest of keeping the API sane and keeping our release schedule, > we're dropping the feature for specifying channel variables in the > origination request. > > We will bring the feature back soon, as a backward compatible addition > to the API. > > > Diffs > - > > /branches/12/rest-api/api-docs/channels.json 404507 > /branches/12/res/res_ari_channels.c 404507 > /branches/12/res/ari/resource_channels.c 404507 > /branches/12/res/ari/resource_channels.h 404507 > /branches/12/res/ari/resource_applications.h 404507 > > Diff: https://reviewboard.asterisk.org/r/3088/diff/ > > > Testing > --- > > In process. > > > Thanks, > > David Lee > > -- _ -- 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] 3088: ari: Remove support for specifying channel vars during origination.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3088/ --- Review request for Asterisk Developers and Matt Jordan. Bugs: ASTERISK-23051 https://issues.asterisk.org/jira/browse/ASTERISK-23051 Repository: Asterisk Description --- When we added support for specifying channel variables for an origination, we didn't consider how that would interact with another feature, namely specifying request parameters in a JSON request body. The method of specifying channel variables (as a flat JSON object passed in the JSON body) interferes with parsing parameters out of the request body. Unfortunately, fixing this would be a backward incompatible change. In the interest of keeping the API sane and keeping our release schedule, we're dropping the feature for specifying channel variables in the origination request. We will bring the feature back soon, as a backward compatible addition to the API. Diffs - /branches/12/rest-api/api-docs/channels.json 404507 /branches/12/res/res_ari_channels.c 404507 /branches/12/res/ari/resource_channels.c 404507 /branches/12/res/ari/resource_channels.h 404507 /branches/12/res/ari/resource_applications.h 404507 Diff: https://reviewboard.asterisk.org/r/3088/diff/ Testing --- In process. Thanks, David Lee -- _ -- 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] 2993: testsuite: Tests for form and JSON request bodies with ARI
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2993/ --- (Updated Nov. 27, 2013, 9:51 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 4379 Bugs: ASTERISK-22685 and ASTERISK-22743 https://issues.asterisk.org/jira/browse/ASTERISK-22685 https://issues.asterisk.org/jira/browse/ASTERISK-22743 Repository: testsuite Description --- This patch adds testsuite tests for form request bodies (content type application/x-www-form-urlencoded) and JSON request bodies. Since we're testing lower level ARI functionality, this is a test using a simple Python script using requests to hit the API directly, instead of our ARI test suite abstraction. Diffs - /asterisk/trunk/tests/rest_api/tests.yaml 4325 /asterisk/team/dlee/ari-req-bodies/tests/rest_api/request-bodies/test-config.yaml PRE-CREATION /asterisk/team/dlee/ari-req-bodies/tests/rest_api/request-bodies/run-test PRE-CREATION Diff: https://reviewboard.asterisk.org/r/2993/diff/ Testing --- The test passes. See https://reviewboard.asterisk.org/r/2994/ and https://reviewboard.asterisk.org/r/2993/. Thanks, David Lee -- _ -- 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] 2994: ari:Add application/json parameter support
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2994/ --- (Updated Nov. 27, 2013, 9:39 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Paul Belanger. Changes --- Committed in revision 403175 Bugs: ASTERISK-22685 https://issues.asterisk.org/jira/browse/ASTERISK-22685 Repository: Asterisk Description --- The patch allows ARI to parse request parameters from an incoming JSON request body, instead of requiring the request to come in as query parameters (which is just weird for POST and DELETE) or form parameters (which is okay, but a bit asymmetric given that all of our responses are JSON). For any operation that does _not_ have a parameter defined of type body (i.e. "paramType": "body" in the API declaration), if a request provides a request body with a Content type of "application/json", the provided JSON document is parsed and searched for parameters. The expected fields in the provided JSON document should match the query parameters defined for the operation. If the parameter has 'allowMultiple' set, then the field in the JSON document may optionally be an array of values. Diffs - /branches/12/tests/test_ari.c 402455 /branches/12/rest-api-templates/swagger_model.py 402455 /branches/12/rest-api-templates/res_ari_resource.c.mustache 402455 /branches/12/rest-api-templates/param_parsing.mustache 402455 /branches/12/rest-api-templates/asterisk_processor.py 402455 /branches/12/res/res_ari_sounds.c 402455 /branches/12/res/res_ari_recordings.c 402455 /branches/12/res/res_ari_playback.c 402455 /branches/12/res/res_ari_endpoints.c 402455 /branches/12/res/res_ari_channels.c 402455 /branches/12/res/res_ari_bridges.c 402455 /branches/12/res/res_ari_asterisk.c 402455 /branches/12/res/res_ari_applications.c 402455 /branches/12/res/res_ari.c 402455 /branches/12/main/http.c 402455 /branches/12/include/asterisk/http.h 402455 /branches/12/include/asterisk/ari.h 402455 Diff: https://reviewboard.asterisk.org/r/2994/diff/ Testing --- Testsuite test. See https://reviewboard.asterisk.org/r/2993/ Thanks, David Lee -- _ -- 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] 2993: testsuite: Tests for form and JSON request bodies with ARI
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2993/ --- (Updated Nov. 27, 2013, 9:36 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 403175 Bugs: ASTERISK-22685 and ASTERISK-22743 https://issues.asterisk.org/jira/browse/ASTERISK-22685 https://issues.asterisk.org/jira/browse/ASTERISK-22743 Repository: testsuite Description --- This patch adds testsuite tests for form request bodies (content type application/x-www-form-urlencoded) and JSON request bodies. Since we're testing lower level ARI functionality, this is a test using a simple Python script using requests to hit the API directly, instead of our ARI test suite abstraction. Diffs - /asterisk/trunk/tests/rest_api/tests.yaml 4325 /asterisk/team/dlee/ari-req-bodies/tests/rest_api/request-bodies/test-config.yaml PRE-CREATION /asterisk/team/dlee/ari-req-bodies/tests/rest_api/request-bodies/run-test PRE-CREATION Diff: https://reviewboard.asterisk.org/r/2993/diff/ Testing --- The test passes. See https://reviewboard.asterisk.org/r/2994/ and https://reviewboard.asterisk.org/r/2993/. Thanks, David Lee -- _ -- 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] 3025: ARI: Implement device state API
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3025/#review10257
---
branches/12/include/asterisk/stasis_app.h
<https://reviewboard.asterisk.org/r/3025/#comment19612>
This was declared above.
branches/12/main/devicestate.c
<https://reviewboard.asterisk.org/r/3025/#comment19614>
Unused.
branches/12/res/ari.make
<https://reviewboard.asterisk.org/r/3025/#comment19617>
device_states should be snake_case instead of camelCase. That's my bad in
the generator.
Fixed on 12 in r402981. Merge from the latest 12, run make ari-stubs, and
rename your files to snake_case.
Sorry about that.
branches/12/res/ari/resource_deviceStates.c
<https://reviewboard.asterisk.org/r/3025/#comment19618>
Isn't a 404 Not Found also possible here?
branches/12/res/ari/resource_deviceStates.c
<https://reviewboard.asterisk.org/r/3025/#comment19619>
This should be a 409 Conflict, since it's a user error (wrong sort of
device state) and not a server side error.
branches/12/res/ari/resource_deviceStates.c
<https://reviewboard.asterisk.org/r/3025/#comment19621>
1. I would have thought that missing would be a 404, and unknown a 500
2. You have a bad fall through to the default case.
3. You shouldn't be using default anyways. Be specific, so when new
response codes are added, GCC will tell us about missing cases in dev mode.
branches/12/res/ari/resource_deviceStates.c
<https://reviewboard.asterisk.org/r/3025/#comment19622>
409
branches/12/res/ari/resource_deviceStates.c
<https://reviewboard.asterisk.org/r/3025/#comment19623>
Similar comment as above.
branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3025/#comment19615>
No sense having both app_name and stasis_app_name. Just
s/app_name/stasis_app_name/ and make it public.
branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3025/#comment19616>
Since res_stasis registers its own event sources, you might have a module
reference problem here. It will bump its own reference count during module
load. Since the unload won't run until the reference count goes back down to
zero, and the unregister happens during the unload process, much sadness.
Mostly good. Just a few problems.
- David Lee
On Nov. 21, 2013, 1:12 p.m., Kevin Harwell wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3025/
> -------
>
> (Updated Nov. 21, 2013, 1:12 p.m.)
>
>
> Review request for Asterisk Developers, David Lee and Matt Jordan.
>
>
> Bugs: ASTERISK-22838
> https://issues.asterisk.org/jira/browse/ASTERISK-22838
>
>
> Repository: Asterisk
>
>
> Description
> ---
>
> Created a data model and implemented functionality for an ARI device state
> resource. The following operations have been added that allow a user to
> manipulate an ARI controlled device:
>
> PUT/device-states/{deviceName}&{deviceState} - Create/Change the state of
> an ARI controlled device
> GET/device-states/{deviceName} - Retrieve the current state of a device
> DELETE /device-states/{deviceName} - Destroy a device-state controlled by ARI
>
> The ARI controlled device must begin with 'Stasis:'. An example controlled
> device name would be Stasis:Example.
>
> A 'DeviceStateChanged' event has also been added so that an application can
> subscribe and receive device change events. Any device state, ARI controlled
> or not, can be subscribed to.
>
> While adding the event, the underlying subscription control mechanism was
> refactored so that all current and future resource subscriptions would be the
> same. Each event resource must now register itself in order to be able to
> properly handle [un]subscribes.
>
>
> Diffs
> -
>
> branches/12/rest-api/resources.json 402955
> branches/12/rest-api/api-docs/events.json 402955
> branches/12/rest-api/api-docs/deviceStates.json PRE-CREATION
> branches/12/rest-api/api-docs/applications.json 402955
> branches/12/res/stasis/app.c 402955
> branches/12/res/res_stasis_device_state.exports.in PRE-CREATION
> branches/12/res/res_stasis_device_state.c PRE-CREATION
> branches/12/res/res_stasis.c 402955
> branches/12/res/res_ari_deviceStates.c PRE-CREATION
> branches/12/res/ari/resource_deviceStates.c PRE-CREATION
> branches/12/res/ari/resource_deviceStates.h PRE-CREATION
> branches/12/res/ar
Re: [asterisk-dev] [Code Review] 2994: ari:Add application/json parameter support
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2994/ --- (Updated Nov. 21, 2013, 2:47 p.m.) Review request for Asterisk Developers and Paul Belanger. Changes --- Properly handle allocations when both query params and JSON body params are offered. Bugs: ASTERISK-22685 https://issues.asterisk.org/jira/browse/ASTERISK-22685 Repository: Asterisk Description --- The patch allows ARI to parse request parameters from an incoming JSON request body, instead of requiring the request to come in as query parameters (which is just weird for POST and DELETE) or form parameters (which is okay, but a bit asymmetric given that all of our responses are JSON). For any operation that does _not_ have a parameter defined of type body (i.e. "paramType": "body" in the API declaration), if a request provides a request body with a Content type of "application/json", the provided JSON document is parsed and searched for parameters. The expected fields in the provided JSON document should match the query parameters defined for the operation. If the parameter has 'allowMultiple' set, then the field in the JSON document may optionally be an array of values. Diffs (updated) - /branches/12/tests/test_ari.c 402455 /branches/12/rest-api-templates/swagger_model.py 402455 /branches/12/rest-api-templates/res_ari_resource.c.mustache 402455 /branches/12/rest-api-templates/param_parsing.mustache 402455 /branches/12/rest-api-templates/asterisk_processor.py 402455 /branches/12/res/res_ari_sounds.c 402455 /branches/12/res/res_ari_recordings.c 402455 /branches/12/res/res_ari_playback.c 402455 /branches/12/res/res_ari_endpoints.c 402455 /branches/12/res/res_ari_channels.c 402455 /branches/12/res/res_ari_bridges.c 402455 /branches/12/res/res_ari_asterisk.c 402455 /branches/12/res/res_ari_applications.c 402455 /branches/12/res/res_ari.c 402455 /branches/12/main/http.c 402455 /branches/12/include/asterisk/http.h 402455 /branches/12/include/asterisk/ari.h 402455 Diff: https://reviewboard.asterisk.org/r/2994/diff/ Testing --- Testsuite test. See https://reviewboard.asterisk.org/r/2993/ Thanks, David Lee -- _ -- 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] 2994: ari:Add application/json parameter support
> On Nov. 20, 2013, 6:07 p.m., Matt Jordan wrote: > > /branches/12/main/http.c, line 631 > > <https://reviewboard.asterisk.org/r/2994/diff/2/?file=48312#file48312line631> > > > > Particularly since this is coming from an external source, we should > > probably use sscanf. > > > > You pretty much do all of your checking for this already in > > ast_http_get_json, so it might be a moot point, but hey - coding guidelines > > :-) > > Matt Jordan wrote: > Of course, this is auto-generated... At the time, the handler didn't have a great way of handling parse failures anyways, so atoi wasn't any different than using sscanf. Now I can handle failures, so we could exit early with a 400 if someone gives a string for a numeric input. But that's a change to the code generator, and outside of scope for this patch. - David --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2994/#review10223 ------- On Nov. 15, 2013, 12:29 p.m., David Lee wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/2994/ > --- > > (Updated Nov. 15, 2013, 12:29 p.m.) > > > Review request for Asterisk Developers and Paul Belanger. > > > Bugs: ASTERISK-22685 > https://issues.asterisk.org/jira/browse/ASTERISK-22685 > > > Repository: Asterisk > > > Description > --- > > The patch allows ARI to parse request parameters from an incoming JSON > request body, instead of requiring the request to come in as query > parameters (which is just weird for POST and DELETE) or form > parameters (which is okay, but a bit asymmetric given that all of our > responses are JSON). > > For any operation that does _not_ have a parameter defined of type > body (i.e. "paramType": "body" in the API declaration), if a request > provides a request body with a Content type of "application/json", the > provided JSON document is parsed and searched for parameters. > > The expected fields in the provided JSON document should match the > query parameters defined for the operation. If the parameter has > 'allowMultiple' set, then the field in the JSON document may > optionally be an array of values. > > > Diffs > - > > /branches/12/tests/test_ari.c 402455 > /branches/12/rest-api-templates/swagger_model.py 402455 > /branches/12/rest-api-templates/res_ari_resource.c.mustache 402455 > /branches/12/rest-api-templates/param_parsing.mustache 402455 > /branches/12/rest-api-templates/asterisk_processor.py 402455 > /branches/12/res/res_ari_sounds.c 402455 > /branches/12/res/res_ari_recordings.c 402455 > /branches/12/res/res_ari_playback.c 402455 > /branches/12/res/res_ari_endpoints.c 402455 > /branches/12/res/res_ari_channels.c 402455 > /branches/12/res/res_ari_bridges.c 402455 > /branches/12/res/res_ari_asterisk.c 402455 > /branches/12/res/res_ari_applications.c 402455 > /branches/12/res/res_ari.c 402455 > /branches/12/main/http.c 402455 > /branches/12/include/asterisk/http.h 402455 > /branches/12/include/asterisk/ari.h 402455 > > Diff: https://reviewboard.asterisk.org/r/2994/diff/ > > > Testing > --- > > Testsuite test. > > See https://reviewboard.asterisk.org/r/2993/ > > > Thanks, > > David Lee > > -- _ -- 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] 3019: ari: Add silence generator controls
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3019/ --- (Updated Nov. 21, 2013, 9:55 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers and Joshua Colp. Changes --- Committed in revision 402926 Bugs: ASTERISK-22514 https://issues.asterisk.org/jira/browse/ASTERISK-22514 Repository: Asterisk Description --- This patch adds the ability to start a silence generator on a channel via ARI. This generator will play silence on the channel (avoiding audio timeouts on the peer) until it is stopped, or some other media operation is started (like playing media, starting music on hold, etc.). Diffs - /branches/12/rest-api/api-docs/channels.json 402842 /branches/12/res/stasis/control.c 402842 /branches/12/res/res_ari_channels.c 402842 /branches/12/res/ari/resource_channels.c 402842 /branches/12/res/ari/resource_channels.h 402842 /branches/12/include/asterisk/stasis_app.h 402842 Diff: https://reviewboard.asterisk.org/r/3019/diff/ Testing --- Manual testing. Thanks, David Lee -- _ -- 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
