[asterisk-dev] [Code Review] LibPRI changes for Generic AOC

Wed Apr 14 16:04:16 CDT 2010

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

Since this is the first pass, I did not worry too much about spacing irregularities like the extra leading blank lines at the beginning or end of blocks.

/team/rmudgett/aoc_event/libpri.h
<https://reviewboard.asterisk.org/r/619/#comment3931>

    Shorten these names.  Replace REQUEST with REQ and RESPONSE with RSP.  See CC_STATUS_REQ_RSP.

/team/rmudgett/aoc_event/libpri.h
<https://reviewboard.asterisk.org/r/619/#comment3938>

    Consider shortening PRI_AOC_REQUEST_RESPONSE to PRI_AOC_REQ_RSP.

    Also PRI_AOC_REQUEST_RESPONSE_SPECIAL_ARG should end with ARR (for arrangement) not ARG.

/team/rmudgett/aoc_event/libpri.h
<https://reviewboard.asterisk.org/r/619/#comment3924>

    Add Doxygen function header comments.

/team/rmudgett/aoc_event/libpri.h
<https://reviewboard.asterisk.org/r/619/#comment3941>

    The "const int" in these prototypes serves no purpose except to make the line longer.

/team/rmudgett/aoc_event/libpri.h
<https://reviewboard.asterisk.org/r/619/#comment3929>

    Using the pri_subcmd_aoc_request structure for the parameter does not buy anything since you are only using one element in the structure.

    Also this function should probably not be part of the public API.

/team/rmudgett/aoc_event/libpri.h
<https://reviewboard.asterisk.org/r/619/#comment3956>

    Another parameter needs to be added to specify a bias for units or currency.  These messages should be consistent since the units and currency messages can encode charging info not available and free of charge.

    More discussion is needed as to whether we remember what has happened before or this is a configurable option.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3957>

    An error or reject needs to be sent back instead of defaulting to an AOC-S request.  This needs to be determined before allocating subcmd.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3950>

    Change the parameter type into to the expected enum type.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3949>

    Change the parameter type into to the expected enum type.  Then the compiler would catch your incorrect enum values in the switch.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3947>

    Use aoc_enc_etsi_subcmd_amount() instead of inlineing it here.

    Actually a reverse function to aoc_etsi_subcmd_recorded_currency() would be better.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3952>

    Why not just call your conversion function to determine if it should be included instead.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3927>

    There are helper routines already available for dealing with party number rather than inlining the code.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3955>

    Convert this to the inverse of aoc_etsi_subcmd_recorded_units().

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3953>

    Why not just call your conversion function to determine if it should be included instead.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3928>

    Helper routines are available to deal with party numbers so you do not have to inline the code here.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3954>

    Convert this to the inverse of aoc_etsi_subcmd_recorded_units().

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3951>

    You are comparing the E billing ID instead of the D billing ID here.  Why not just call your conversion function to determine if it should be included instead.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3946>

    Use aoc_enc_etsi_subcmd_amount() instead of inlineing it here.

    Actually a reverse function to aoc_etsi_subcmd_recorded_currency() would be better.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3948>

    You are comparing the E billing ID instead of the D billing ID here.  Why not just call your conversion function to determine if it should be included instead.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3925>

    The switch cases should line up with the switch.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3942>

    Update the parameter list and describe.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3933>

    Returning any other value than 0 or 1 is meaningless.  This particular function should always return 1.

    APDU_CALLBACK_REASON_MSG_REJECT is not explicitly handled as the default in this function.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3932>

    The PRI_MASTER check is not necessary here.  You are getting the result you asked for.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3935>

    Use apdu->response.user.long instead of ptr.  The value is a union designed for being able to pass a simple integer.  Also where this ptr is initialized in aoc_charging_request_encode() is going to have a dangling pointer!

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3937>

    You need to memset() the aoc_request_response structure to zero.  Otherwise the valid_aoc_s flag is uninitialized for most cases.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3936>

    Just eliminate errorcode so static checkers can verify that the switch cases are members of the enum.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3926>

    The AOC request messages need to have a REALLY long timeout or a method to wait indefinitely until a CONNECT message comes in as a timeout.

    The second option would be better.

/team/rmudgett/aoc_event/pri_aoc.c
<https://reviewboard.asterisk.org/r/619/#comment3944>

    Maybe aoc_x_encode() would be a better name series than aoc_aocx_encode().

- rmudgett

On 2010-04-13 17:53:56, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/619/
> -----------------------------------------------------------
> 
> (Updated 2010-04-13 17:53:56)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This review compliments the Asterisk changes in review https://reviewboard.asterisk.org/r/552/.  These are the LibPRI changes required to send ETSI AOC messages generated by Asterisk.
> 
> 
> Diffs
> -----
> 
>   /team/rmudgett/aoc_event/libpri.h 1592 
>   /team/rmudgett/aoc_event/pri_aoc.c 1592 
>   /team/rmudgett/aoc_event/pri_facility.h 1592 
>   /team/rmudgett/aoc_event/pri_facility.c 1592 
>   /team/rmudgett/aoc_event/pri_internal.h 1592 
>   /team/rmudgett/aoc_event/q931.c 1592 
> 
> Diff: https://reviewboard.asterisk.org/r/619/diff
> 
> 
> Testing
> -------
> 
> These routines have been tested back to back with Asterisk, but I am going to need some community help to test interoperability.
> 
> To test this code, check out both my Asterisk and Libpri changes for AOC
> 
> Asterisk Changes: svn/asterisk/team/dvossel/generic_aoc
> LibPRI Changes: svn/libpri/team/dvossel/aoc_send 
> 
> 
> Thanks,
> 
> David
> 
>