[asterisk-dev] [Code Review]: WebSocket HTTP Module

[Joshua Colp]

> On May 30, 2012, 11:35 a.m., Mark Michelson wrote:
> > /trunk/res/res_http_websocket.c, lines 227-231
> > <https://reviewboard.asterisk.org/r/1952/diff/1/?file=28385#file28385line227>
> >
> >     session->reconstruct = MIN(bytes, MAXIMUM_RECONSTRUCTION_CEILING);

Fixed.


> On May 30, 2012, 11:35 a.m., Mark Michelson wrote:
> > /trunk/res/res_http_websocket.c, line 150
> > <https://reviewboard.asterisk.org/r/1952/diff/1/?file=28385#file28385line150>
> >
> >     I don't understand the inclusion of "callback" as a parameter to this function. Does this mean that it's acceptable to have multiple protocols with the same name but different callbacks registered? If that's the case, then the comparison callback for the container needs to always take into consideration the callback parameter.

It's a sanity check essentially to ensure other modules don't try to register your sub-protocol.


> On May 30, 2012, 11:35 a.m., Mark Michelson wrote:
> > /trunk/res/res_http_websocket.c, line 119
> > <https://reviewboard.asterisk.org/r/1952/diff/1/?file=28385#file28385line119>
> >
> >     This locking seems unnecessary. ao2_find will lock the container while trying to find the desired protocol, and ao2_link() will lock the container while the protocol is added to the container.
> >     
> >     If this locking is required for some other reason, then pass the OBJ_NOLOCK flags to ao2_find and ao2_link.

It ensures that between the time of trying to find out if the sub-protocol is already registered and registering it that no other module registers the same sub-protocol. Will it ever happen? Doubtful but better to be safe than sorry.


> On May 30, 2012, 11:35 a.m., Mark Michelson wrote:
> > /trunk/res/res_http_websocket.c, lines 117-126
> > <https://reviewboard.asterisk.org/r/1952/diff/1/?file=28385#file28385line117>
> >
> >     This certainly isn't something you have to do, but there is an OBJ_KEY attribute you can pass to ao2_find() that allows you to just pass the name of the protocol in instead of having to create a temporary websocket_protocol on the stack. You would have to adjust the hash and comparison functions to accommodate the use of OBJ_KEY if you went this route.

Changed.


- Joshua


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


On May 29, 2012, 1:25 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1952/
> -----------------------------------------------------------
> 
> (Updated May 29, 2012, 1:25 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This adds support for WebSocket protocols 7, 8, and 13. These are the most recent protocols. Sending and receiving text/binary frames is supported along with the various operation codes. An API is provided which makes it easy to implement different sub-protocols. Frame reconstruction is supported for situations where desirable (if multiple frames are received they will be reconstructed into a single one) but this can be disabled in situations where streaming is wanted.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/http_websocket.h PRE-CREATION 
>   /trunk/include/asterisk/utils.h 360141 
>   /trunk/main/utils.c 360141 
>   /trunk/res/res_http_websocket.c PRE-CREATION 
>   /trunk/res/res_http_websocket.exports.in PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1952/diff
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome Canary to confirm connection is established and data can be sent and received.
> 
> Tested using websocket.py to confirm connection is established and data can be sent and received.
> 
> 
> Thanks,
> 
> Joshua
> 
>

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