[asterisk-dev] [Code Review] Generic Advice of Charge

David Vossel dvossel at digium.com
Thu Apr 15 17:15:36 CDT 2010 


> On 2010-04-15 16:35:24, rmudgett wrote:
> > /team/rmudgett/aoc_event/channels/sig_pri.c, lines 4824-4825
> > <https://reviewboard.asterisk.org/r/552/diff/3/?file=9249#file9249line4824>
> >
> >     Move the setting of the waiting_for_aoce into sig_pri_send_aoce_termination_request().  If the routine fails, you would have the flag set and noone disconnecting the call.

This isn't necessary, if the call fails to send the termination request it hangs up the channel and that flag is cleared in sig_pri_hangup... But it is coupled with that function so I agree that it belongs there.


> On 2010-04-15 16:35:24, rmudgett wrote:
> > /team/rmudgett/aoc_event/channels/sig_pri.c, lines 5593-5605
> > <https://reviewboard.asterisk.org/r/552/diff/3/?file=9249#file9249line5593>
> >
> >     You are not parsing the option parameters as described by the review description.  The A() option could be given as A(sde) instead of A(s,d,e).  I suppose that we could document the option as needing the commas.  The comma format could allow a future option list for Q.SIG option names.

right, there is no comma delimiter checked for.  I'd rather just document it as not requiring commas.


> On 2010-04-15 16:35:24, rmudgett wrote:
> > /team/rmudgett/aoc_event/channels/sig_pri.c, lines 5939-5950
> > <https://reviewboard.asterisk.org/r/552/diff/3/?file=9249#file9249line5939>
> >
> >     The use of aoc_passthrough_flag seems odd here.  It was tested to post the AST_CONTROL_AOC and is tested by the receiver of the AST_CONTROL_AOC frame also?

If the channel receives AOC through Asterisk and the corresponding pass-through flag is not set, then it is dropped.   If the channel gets AOC from the pri and the corresponding pass-through flag is not set, then the manager event occurs, but the message stops there and does not get sent to the bridged channel.

So we neither send AOC out the pri nor receive AOC from Asterisk on the channel when the corresponding pass-through flag is not set.  Does that make sense?


> On 2010-04-15 16:35:24, rmudgett wrote:
> > /team/rmudgett/aoc_event/main/aoc.c, line 233
> > <https://reviewboard.asterisk.org/r/552/diff/3/?file=9255#file9255line233>
> >
> >     You are hiding the ie variable in this function.  This is usually not such a good idea.

I don't understand this comment.


> On 2010-04-15 16:35:24, rmudgett wrote:
> > /team/rmudgett/aoc_event/main/aoc.c, lines 249-251
> > <https://reviewboard.asterisk.org/r/552/diff/3/?file=9255#file9255line249>
> >
> >     This decoding of a multi-byte integer is not reliable.  What if len were something other than 4?  A helper routine to decode an integer would be a more robust method.  The ntohl() is only valid if the size is as expected.
> >     
> >     There are integer encode/decode routines in the libpri ASN.1 code which could be extracted and made into utility routines for the Asterisk control frames.

ie.amount is set as a uint32_t.  Does that not guarantee it is 4 bytes?


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/552/#review1842
-----------------------------------------------------------


On 2010-04-13 17:43:42, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/552/
> -----------------------------------------------------------
> 
> (Updated 2010-04-13 17:43:42)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Asterisk Generic AOC Representation
> - Generic AOC encode/decode routines. (Generic AOC must be encoded to be passed on the wire in the AST_CONTROL_AOC frame)
> - AST_CONTROL_AOC frame type to represent generic encoded AOC data
> - Manager events for AOC-S, AOC-D, and AOC-E messages
> 
> Asterisk App Support
> - app_dial AOC-S pass-through support on call setup
> - app_queue AOC-S pass-through support on call setup
> 
> AOC Unit Tests
> - AOC Unit Tests for encode/decode routines
> - AOC Unit Test for manager event representation.
> 
> SIP AOC Support
> - Pass-through of generic AOC-D and AOC-E messages to snom phones via the snom AOC specification.
> - Creation of chan_sip page3 flags for the addition of the new 'snom_aoc_enabled' sip.conf option.
> 
> IAX AOC Support
> - Natively supports AOC pass-through through the use of the new AST_CONTROL_AOC frame type
> 
> DAHDI AOC Support
> - ETSI PRI full AOC Pass-through support
> - 'aoc_enable' chan_dahdi.conf option for independently enabling pass-through of AOC-S, AOC-D, AOC-E.
> - 'aoce_delayhangup' option for retrieving AOC-E on disconnect.
> - DAHDI A() dial string option for requesting AOC services.
>   example usage: exten=>1111,1,Dial(DAHDI/g1/1112/A(s,d,e)) ;requests AOC-S, AOC-D, and AOC-E on call setup
> 
> 
> Diffs
> -----
> 
>   /team/rmudgett/aoc_event/apps/app_dial.c 257246 
>   /team/rmudgett/aoc_event/apps/app_queue.c 257246 
>   /team/rmudgett/aoc_event/channels/chan_dahdi.c 257246 
>   /team/rmudgett/aoc_event/channels/chan_sip.c 257246 
>   /team/rmudgett/aoc_event/channels/sig_pri.h 257246 
>   /team/rmudgett/aoc_event/channels/sig_pri.c 257246 
>   /team/rmudgett/aoc_event/channels/sip/include/sip.h 257246 
>   /team/rmudgett/aoc_event/configs/chan_dahdi.conf.sample 257246 
>   /team/rmudgett/aoc_event/configs/sip.conf.sample 257246 
>   /team/rmudgett/aoc_event/include/asterisk/aoc.h PRE-CREATION 
>   /team/rmudgett/aoc_event/include/asterisk/frame.h 257246 
>   /team/rmudgett/aoc_event/main/aoc.c PRE-CREATION 
>   /team/rmudgett/aoc_event/main/asterisk.c 257246 
>   /team/rmudgett/aoc_event/main/channel.c 257246 
>   /team/rmudgett/aoc_event/main/features.c 257246 
>   /team/rmudgett/aoc_event/main/manager.c 257246 
>   /team/rmudgett/aoc_event/tests/test_aoc.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/552/diff
> 
> 
> Testing
> -------
> 
> I have tested every feature listed in the description to some extent, but only back to back with Asterisk.   I am going to need some community help to test interoperability.
> 
> To test this code, check out both my Asterisk and Libpri changes.
> 
> Asterisk Changes: svn/asterisk/team/dvossel/generic_aoc
> LibPRI Changes: svn/libpri/team/dvossel/aoc_send 
> 
> 
> Thanks,
> 
> David
> 
>

More information about the asterisk-dev mailing list