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

Thu Oct 15 12:47:42 CDT 2009

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


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

    The only changes left in chan_dahdi.c are changes to blank lines.  Reverting them will thus eliminate chan_dahdi.c from the diff.


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

    Changing this test to if (pri_subaddress->length <= 0) would guard against bad length values.


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

    Changing this declaration to
    char *cnum = ast_malloca(2 * pri_subaddress->length + 1);
    would eliminate the need for an over generous array size.


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

    Doxygen function comment.
    The dst is not an ASCIIz string.
    The src is an ASCIIz hex string.
    
    You may want to return the number of bytes used by dst which would be (strlen(src) / 2 + strlen(src) % 2).  The place where this function is called could use the returned length.


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

    It is possible for ast_subaddress->str to be NULL here.
    
    Also don't set the length here as the string may be truncated or reduced when packing a hex string.


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

    Use ast_copy_string here and set the length after the copy in case the string was truncated.
    

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

    Packing a hex string usually halves the resulting length.


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

    The else clause is not needed because the function assumes that pri_subaddress has been previously memset to zero.


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

    This line in sig_pri_party_subaddress_from_ast() is using spaces to indent.


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

    This should be the channel lock: sig_pri_lock_owner(pri, chanpos)
    
    You are changing the ast_channel structure in sig_pri_set_subaddress() here.


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

    To be consistent with how the original code worked, you should also check the subaddress length before setting the channel variable.


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

    Use ast_channel_unlock(c) here since c is also the owner pointer here.


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

    This should be the channel lock: sig_pri_lock_owner(pri, chanpos)


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

    Check the subaddress length here too.


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

    Use ast_channel_unlock(c) here since c is also the owner pointer here.


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

    The original code tested if the subaddress string was not empty.


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

    #if defined(HAVE_PRI_SUBADDR)
    <new code>
    #else
    <original code>
    #endif /* !defined(HAVE_PRI_SUBADDR)
    
    See similar earlier section of code.  Usually what is done to one section should also be done to the other.
    
    You need to use sig_pri_lock_onwer() since you are changing the channel structure.


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

    Call ast_party_subaddress_init() here instead.


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

    If you put the switch default between the two lines here it will eliminate some redundancy.


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

    The else clause not needed if ast_party_subaddress_init() called above.


- rmudgett


On 2009-10-15 04:49:50, alecdavis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/405/
> -----------------------------------------------------------
> 
> (Updated 2009-10-15 04:49:50)
> 
> 
> 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
> 
>