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

rmudgett at digium.com rmudgett at digium.com
Thu Oct 15 15:10:25 CDT 2009 

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


Please update your asterisk and libpri working copies.  The asterisk patch no longer applies cleanly and the libpri patch has a conflict.


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

    If calling_subaddress were redefined to:
    struct pri_party_id calling;
    it would bring out all available information about the caller that may be needed in the future.  Also there already exists a function that will fill in the structure.



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

    Add a comment to remove this line and put a test for the feature in configure.ac when it is committed.



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

    API call not needed.  The pri_sr_set_caller_party() can already set this information.



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

    The size of the q931_subaddress->data[] and the pri_subaddress->data[] are different.  You need to add checks to truncate the data.  Also if you need to truncate, the odd_even_indicator should be cleared since it will always be even.



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

    Need to call pri_copy_party_subaddress_to_q931() here as well.  Subaddress is part of the structure.



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

    This API call is not needed.  The pri_sr_set_caller_party() already does this as part of setting the party id.



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

    A better place for this function is right after pri_sr_set_called() since both functions are dealing with the called party.
    
    This is mostly for esthetics.



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

    This function does not do what it claims to do.  If someone were to call this function to sort the subaddresses it would not work.  Currently, everyone who is calling it is only interested in if there is a difference.
    
    Sorting comparison priority should be:
    type
    subaddress data contents to min(left->length, right->length)
    length
    odd_even_indicator
    
    Also you must use memcmp() instead of strcmp().



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

    Add a comment that the size of the pri_subaddress->data[] is not the same as the size of q931_subaddress->data[].
    
    Currently the q931 size is smaller than the pri size so this code will always work.



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

    You need to set all elements of the structure here.



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

    This function does not handle the case if len is zero.



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

    Create a generic dump subaddress helper function that also takes the name of the subaddress being dumped.  (Called, Calling, Redirecting, Connected)



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

    The conditional code can be removed since that is what q931_get_subaddr_specific() is for.



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

    Same here.



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

    Create a generic receive subaddress helper function that also takes a pointer to the subaddress being received.  (Called, Calling, Redirecting, Connected)



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

    Just shift and mask the bit field here.



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

    Create a generic transmit subaddress helper function that also takes a pointer to the subaddress being sent.  (Called, Calling, Redirecting, Connected)



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

    Delete these lines.  Asterisk is supposed to do this.



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

    With the generic subaddres helper functions, these connected subaddress functions will be trivial.  Also doing these functions will allow connected line with subaddress for the initial connect to work.  (Call transfers would require a lot more work.)
    
    Also, add Q931_IE_CONNECTED_SUBADDR to connect_ies[].



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

    This change is now in conflict with current checked in code.



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

    Delete this function.  It is a duplicate of q931_party_subaddress_copy_to_pri().  There are several references later that will need to use the correct function.



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

    Along with changing ev.ring.calling_subaddress to ev.ring.calling, call q931_party_id_copy_to_pri() here.



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

    You need to set callingsubaddr[0] = 0 if type is not NSAP.  Earlier code will break otherwise.



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

    Along with changing ev.ring.calling_subaddress to ev.ring.calling, call q931_party_id_copy_to_pri() here.
    
    You need to set callingsubaddr[0] = 0 if type is not NSAP.  Earlier code could break otherwise.


- rmudgett


On 2009-10-15 05:15:46, alecdavis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/406/
> -----------------------------------------------------------
> 
> (Updated 2009-10-15 05:15: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 1166 
>   /team/rmudgett/subaddr/pri.c 1166 
>   /team/rmudgett/subaddr/pri_internal.h 1166 
>   /team/rmudgett/subaddr/pri_q931.h 1166 
>   /team/rmudgett/subaddr/q931.c 1166 
> 
> Diff: https://reviewboard.asterisk.org/r/406/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> alecdavis
> 
>

More information about the asterisk-dev mailing list