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

Thu Apr 15 16:35:24 CDT 2010

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

Next up looking at aoc.c, manager.c, and test_aoc.c.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3973>

    Update the param list.  passthrough is not documented.

    Also there are three locks already obtained.  The pri->lock, sig_pri private lock, and the owner channel lock.

    There are other new functions that need doxygen updates.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3982>

    You have extraneous semicolons on the curly braces of many switch statements.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3979>

    The number and type can be unavailable in four combinations.
    1) The case where they are both not available should be deleted/skipped.
    2) The case where one or the other is not available should be handled.  Neither of these cases are handled very well.
    3) The case where both are available.

    This is the same in sig_pri_aoc_e_from_pri().  Actually this should be put in a helper routine since it is dealing with the same structure in both places.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3983>

    This can be assigned idx after the for loop rather than incrementing it each time through the loop.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3984>

    A helper routine for currency could be done here and also used for the E routine.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3985>

    A helper routine for units could be done here and also used for the E routine.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3986>

    Assign i to this after the for loop so you do not need to increment every time through the loop.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3987>

    Please look closer at the libpri aoc subcmd structs.  The charging association id is a 16 bit signed value thus zero is valid.

    Charging association is a ternary value:
    1) It is not present.
    2) It is an ID number.
    3) It is a party number.

    The AOC frame needs to include the above meta data with any charging association data.

    FYI, I expect the charging association data to be present only if the AOC-E comes in on the dummy channel.  i.e., Not associated with a current call.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3990>

    This call can fail.  In this case it could have bad consequences.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3974>

    A response must always be given here.  The failure cases are not handled.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3992>

    A hangup with and without AOC-E being received needs to be tried.

    Is there a way to determine if an AOC-E is really expected?  If it is not expected, then we will not need to do the delay.

    I think if the delay option is set we are expecting the AOC-E message and the delay is needed.

    If the AOC-E is specifically requested by the caller and the delay option is not enabled should we wait anyway?  Maybe we could make the delay option be: never, on_request, always.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3988>

    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.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3989>

    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.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3972>

    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.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3993>

    The conditional should be starting at the beginning of the line.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3995>

    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?

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3991>

    Swapping these two lines will minimize the time between the test and set.  It might even generate better code.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3994>

    The conditional should be starting at the beginning of the line.

/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment3996>

    The deletion of the blank line here was unnecessary.

/team/rmudgett/aoc_event/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment3975>

    You are hiding the ie variable in this function.  This is usually not such a good idea.

/team/rmudgett/aoc_event/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment3978>

    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.

- rmudgett

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
> 
>