[asterisk-dev] [Code Review] Add Calling and Called subaddress support for Asterisk apps and funcs

rmudgett at digium.com rmudgett at digium.com
Wed Oct 14 16:54:44 CDT 2009 

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



/team/rmudgett/subaddr/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/405/#comment2651>

    Remove this code.



/team/rmudgett/subaddr/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/405/#comment2652>

    Remove this code.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2653>

    Doxygen function comment is needed.
    
    A better place to put this function is right after overall_ast_presentation().



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2678>

    All ast_party_subaddress fields need to be accounted for here.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2679>

    This does not need to be unsigned char.  In fact everywhere you are referencing it you are casting it to char.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2677>

    This code does not handle pri_subaddress->length == 0 very well.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2680>

    Moving these two lines to after the else clause will satisfy my earlier comment in the if clause about setting all of the struct fields.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2654>

    This function needs to be able to convert hex and should be renamed to something like ast_pri_pack_hex_string.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2655>

    This function needs to call sig_pri_party_subaddress_from_ast() because a party id contains a subaddress now.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2656>

    You need to acquire the channel lock before calling sig_pri_set_subaddress().
    
    Also for backwards compatibility, the old CALLINGSUBADDR code needs to be kept in conditional code:
    #if defined(HAVE_PRI_SUBADDR)
    <new code>
    #else
    <old code>
    #endif /* !defined(HAVE_PRI_SUBADDR) */



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2657>

    Same here.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2658>

    Same here.  And the CALLINGSUBADDR conditional code needs to be here as well.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2659>

    Same here.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2660>

    This will need to be removed when done.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2663>

    The dialed_subaddress.odd_even_indicator field is not initialized.  Also if the subaddress is not specified in the dial string, dialed_subaddress is not even initialized.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2661>

    It would be nice to explicitly specify the break statement here.



/team/rmudgett/subaddr/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/405/#comment2662>

    More debug code.



/team/rmudgett/subaddr/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/405/#comment2664>

    Delete this and all code using it.  It does not belong in the ast_party_id structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2665>

    Make this function public.  Prototype it in channel.h.  Use it in sig_pri.c when initializing any ast_party_subaddress variables.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2666>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2667>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2668>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2669>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2670>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2671>

    Group setting the id.subaddress with the rest of the id element initialization.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2672>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2673>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2674>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2675>

    Delete.  Not part of structure.



/team/rmudgett/subaddr/main/channel.c
<https://reviewboard.asterisk.org/r/405/#comment2676>

    Delete.  Not part of structure.


- rmudgett


On 2009-10-14 02:03:56, alecdavis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/405/
> -----------------------------------------------------------
> 
> (Updated 2009-10-14 02:03:56)
> 
> 
> Review request for Asterisk Developers, rmudgett and mattf.
> 
> 
> Summary
> -------
> 
> Add dialstring support for called subaddress, used like Dial(dahdi/g0/5551234:8512) where 5551234 is the main line number of the remote site and 8512 is the extension with that site.
> If 'User Specific' BCD encoding is required, the subaddress is prefixed with a 'U'. IE Dial(dahdi/g0/5551234:U8512)
> 
> Extend function CALLERID() for type 'subaddr' to allow reading and updating the calling subaddress.
> Extend function CALLERID() for type 'dnid-subaddr' to allow reading and updating the called subaddress.
> Extend function CONNECTEDLINE as for CALLERID
> 
> 
> This addresses bug 15604.
>     https://issues.asterisk.org/view.php?id=15604
> 
> 
> Diffs
> -----
> 
>   /team/rmudgett/subaddr/channels/chan_dahdi.c 223051 
>   /team/rmudgett/subaddr/channels/sig_pri.c 223051 
>   /team/rmudgett/subaddr/funcs/func_callerid.c 223051 
>   /team/rmudgett/subaddr/funcs/func_connectedline.c 223051 
>   /team/rmudgett/subaddr/include/asterisk/channel.h 223051 
>   /team/rmudgett/subaddr/main/channel.c 223051 
> 
> Diff: https://reviewboard.asterisk.org/r/405/diff
> 
> 
> Testing
> -------
> 
> Preliminary concept testing between sites over Telecom NZ PSTN, using channel variables $CALLINGSUBADDR and $CALLEDSUBADDR with patches for branches at https://issues.asterisk.org/view.php?id=15604 
> Development and testing using Jtec 5015 switches, establish calls in either direction, NSAP and User Specific.
> 
> 
> Thanks,
> 
> alecdavis
> 
>

More information about the asterisk-dev mailing list