[asterisk-dev] [Code Review] Add support for Cisco USECALLMANAGER Phones and Broadsoft Application Server Events.

[Matt Jordan]

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


First, thanks for contributing this patch.  Its obvious that you've done a lot of work on it, and that a large number of people have tested it and are using it successfully.

Before this review goes forward, however, I think two questions need to be answered conclusively:

1) As your review states, the USECALLMANAGER XML for Cisco phones is a Cisco proprietary API.  Are there any licensing concerns with putting support for this in Asterisk?

2) We have run into issues with the licensing of Broadsoft SIP extensions in the past.  We would need confirmation that the Broadsoft Application Server Events are acceptable in Asterisk as well.

Assuming that those two questions are answered in the affirmative, in order for this review to be manageable, I'd prefer to split it up into logical chunks.  Otherwise, I'm afraid that the sheer size and scope will make it difficult to get it all done in a coherent fashion.

I'd propose that it gets done in three separate reviews:

1) The general 'cleanup' work that was done that is independent of Cisco USECALLMANAGER and the Broadsoft Application Server Events

2) The support for Cisco USECALLMANAGER

3) The support for Broadsoft Application Server Events

If you'd like to break this up in a different fashion that'd be great - but I think keeping it as one monolithic review will prevent greatly hinder its inclusion due to the sheer size and scope.

- Matt


On June 7, 2012, 12:10 a.m., gareth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1976/
> -----------------------------------------------------------
> 
> (Updated June 7, 2012, 12:10 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds support for Cisco USECALLMANAGER phones proprietary XML for extension state subscriptions, server-side call DND/call-forwarding synchronisation, and divert ringing call (iDivert) via REFER as well as reboot and restart via NOTIFY.
> 
> USECALLMANAGER is the keyword required in the <proxy /> element for those phones to function, so the option to enable this behaviour is called cisco_usecallmanager.
> 
> Support for Broadsoft Application Server Events (DND and call-forward) was included because it makes a large chunk of the DND/call-forward code vendor independent. There isn't any Asterisk configuration required to enable this.
> 
> Cisco/SPA, Polycom, SNOM, Aastra, Yealink and Grandstream phones support Broadsoft Application Server Events. 
> 
> * User visible changes -
> 
> - Added new device state DONOTDISTURB.
> 
> - Added new sip peer option cisco_usecallmanager which enables various protocol extensions that the Cisco phones need when running in USECALLMANAGER mode.
> 
> - Added dndbusy option that automatically queues a busy signal when peer that has donotdisturb enabled is called instead of offering it to the peer.
> 
> - Added new SIPPEER() option donotdisturb, either yes or no. Writing to it will also notify peer of the new donotdisturb state.
> 
> - Added new SIPPEER() option callforward, contains the current forwarding extension. Writing to it will also notify the peer of the new forwarding extension.
> 
> - Added sip donotdisturb {on|off} <peer> command that will toggle the do not disturb state of a SIP peer.
> 
> - Added sip callforward {on|off} <peer> [extension] command that will either set or remove the call forwarding extension for a SIP peer.
> 
> - DoNotDisturb and CallForward state are stored in astdb and will persist across asterisk restarts. Those current state of those settings will also be automatically sent to the peer when it registers.
> 
> - Added new SIPPEER() option regcallid, contains the SIP call-id used when the device registered. Used by Cisco USECALLMANAGER phones for SIP NOTIFY requests.
> 
> - sip_notify.conf now supports ${} parsing. PEERNAME variable will contain the name of the peer being sent the notification.
> 
> - Added cisco-restart and cisco-reset types to sip_notify.conf that can be used to restart (reloads line keys, dial-plan and soft keys) or reset cisco_usecallmanager phones.
> 
> - Added support for Broadsoft Application Server feature events. Events supported are SetDoNotDisturb and SetForwarding with a forwardingType of forwardImmediate.
> 
> * Code changes & cleanup in chan_sip -
> 
> - struct sip_refer converted to use the stringfields API.
> 
> - s/allocate/alloc to match other *alloc functions.
> 
> - Replace get_msg_text, get_msg_text2 and get_pidf_body -> No, not get_pidf_msg_text_body3 but get_content, to match add_content.
> 
> - get_body doesn't get the request body, renamed to get_content_line.
> 
> - get_body_by_line doesn't get the body line, and is just a simple if test. Moved code inline and removed function.
> 
> - Remove camelCase in struct sip_peer peer state variables, onHold -> onhold, inUse -> inuse, inRinging -> ringing.
> 
> - Rename randdata in struct sip_pvt to nonce because that is what it is, no need to update struct sip_pvt because _it already has a nonce field_.
> 
> - Remove useless (and inconsistent) 'header' suffix on variables in handle_request_subscribe.
> 
> - Use ast_strdupa on Event header in handle_request_subscribe to avoid overly complicated strncmp calls to find the event package.
> 
> - Move get_destination check in handle_request_subscribe to avoid duplicate checking for packages that don't need it.
> 
> - Use an alloca'd string for the event package in handle_request_subscribe to avoid massively over-complicated strncmp calls.
> 
> - Rename append_date -> add_date to match other add_xxx functions.
> 
> - Added add_expires helper function.
> 
> - Remove _header suffix on add_diversion_header (no other header adding functions have this).
> 
> - Don't pass req->ignore to request handlers if req is also being passed.
> 
> - Don't create a subscription in handle_request_subscribe if p->expiry == 0.
> 
> - Don't walk of the back of referred_by_name when splitting string in get_refer_info
> 
> 
> This addresses bug ASTERISK-13145.
>     https://issues.asterisk.org/jira/browse/ASTERISK-13145
> 
> 
> Diffs
> -----
> 
>   /trunk/UPGRADE.txt 368662 
>   /trunk/channels/chan_sip.c 368662 
>   /trunk/CHANGES 368662 
>   /trunk/channels/sip/include/sip.h 368662 
>   /trunk/channels/sip/security_events.c 368662 
>   /trunk/configs/sip_notify.conf.sample 368662 
>   /trunk/funcs/func_devstate.c 368662 
>   /trunk/include/asterisk/devicestate.h 368662 
>   /trunk/include/asterisk/pbx.h 368662 
>   /trunk/main/devicestate.c 368662 
>   /trunk/main/pbx.c 368662 
> 
> Diff: https://reviewboard.asterisk.org/r/1976/diff
> 
> 
> Testing
> -------
> 
> Tested using various 1.8.x and 10.x branches, forward-ported the 10.x version patch to trunk.
> 
> 
> Thanks,
> 
> gareth
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120614/7354b6da/attachment.htm>