[asterisk-dev] [Code Review] Add ISDN Calling and Called Subaddress support functions to LIBPRI

[rmudgett at digium.com]

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



/team/rmudgett/subaddr/libpri.h
<https://reviewboard.asterisk.org/r/406/#comment2737>

    I'm thinking we need to change this to unsigned char to avoid possible sign extension problems when dealing with BCD data.  This is going to mean more casting, but it will also serve as a reminder that this data is not necessarily an ASCIIz string.
    
    One place where sign extension could be a problem is in sig_pri.c:sig_pri_set_subaddress().



/team/rmudgett/subaddr/libpri.h
<https://reviewboard.asterisk.org/r/406/#comment2742>

    Left over whitespace change to undo.



/team/rmudgett/subaddr/pri_internal.h
<https://reviewboard.asterisk.org/r/406/#comment2738>

    I'm thinking we need to change this to unsigned char to avoid possible sign extension problems when dealing with BCD data.  This is going to mean more casting, but it will also serve as a reminder that this data is not necessarily an ASCIIz string.
    
    There is one case I have seen where an existing cast could then be removed.  q931.c:receive_subaddr_helper()



/team/rmudgett/subaddr/q931.c
<https://reviewboard.asterisk.org/r/406/#comment2743>

    A latent problem here is that maxlen is not used and could be a source of buffer overrun as a result.  I don't think that this is a serious problem, but it should be commented upon at a minimum.  Malformed data coming in on the line when you are doing a dump could cause this overrun.



/team/rmudgett/subaddr/q931.c
<https://reviewboard.asterisk.org/r/406/#comment2740>

    Yes they are coded the same.  Only the ie type identifying whose subaddress is different.  Q.951 Section 5.4.2



/team/rmudgett/subaddr/q931.c
<https://reviewboard.asterisk.org/r/406/#comment2739>

    The comment is no longer necessary or accurate anymore.



/team/rmudgett/subaddr/q931.c
<https://reviewboard.asterisk.org/r/406/#comment2741>

    I think the called and calling subaddress ie's should be put here as well.


- rmudgett


On 2009-10-16 06:54:46, alecdavis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/406/
> -----------------------------------------------------------
> 
> (Updated 2009-10-16 06:54:46)
> 
> 
> Review request for Asterisk Developers, rmudgett and mattf.
> 
> 
> Summary
> -------
> 
> Further Libpri support for subaddress, previously LIBPRI only supported receiving Calling Subaddress.
> 
> The first intentions of this are to add Transmit and Receive capabilites for Calling Subaddress and Called Subaddress.
> 
> Calling, Called and Redirecting Subaddress, now correctly supports User Specific type and the default NSAP.
> 
> Redirection Subaddress, Connectedline Subaddress have been identified, but will come later.
> 
> 
> This addresses bug 15604.
>     http://issues.asterisk.org/view.php?id=15604
> 
> 
> Diffs
> -----
> 
>   /team/rmudgett/subaddr/libpri.h 1218 
>   /team/rmudgett/subaddr/pri.c 1218 
>   /team/rmudgett/subaddr/pri_internal.h 1218 
>   /team/rmudgett/subaddr/pri_q931.h 1218 
>   /team/rmudgett/subaddr/q931.c 1218 
> 
> Diff: https://reviewboard.asterisk.org/r/406/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> alecdavis
> 
>