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

[Joshua Colp]

> On May 30, 2012, 2:46 a.m., wdoekes wrote:
> > /trunk/res/res_http_websocket.c, line 207
> > <https://reviewboard.asterisk.org/r/1952/diff/1/?file=28385#file28385line207>
> >
> >     I was thinking character buffers of >32bit sizes (64 bit actual_length) length were a bit on the large side. But a stack of that size sounds even less plausible.
> >     
> >     For larger payloads I'd expect it to be cheaper to send the frame and the payload in 2 separate fwrite()s instead. If this is an optimization for small payload lengths, then I'd expect a remark to such extent.

Good idea! Changed.


> On May 30, 2012, 2:46 a.m., wdoekes wrote:
> > /trunk/res/res_http_websocket.c, line 289
> > <https://reviewboard.asterisk.org/r/1952/diff/1/?file=28385#file28385line289>
> >
> >     I'd move the mask=&buf[2] to the:
> >     
> >     else if (*payload_len) on line 330, like the other mask settings.
> >     
> >     
> >     Further, I'm not at all intimate with the protocol, but a quick glance rfc6455 hints that mask quad is only present if mask_present==1. The code here reads those 4 bytes (skips from the payload) even if mask_present==0.

Nice catch! All fixed up.


> On May 30, 2012, 2:46 a.m., wdoekes wrote:
> > /trunk/res/res_http_websocket.c, lines 484-485
> > <https://reviewboard.asterisk.org/r/1952/diff/1/?file=28385#file28385line484>
> >
> >     Lots of fprintf()'s in this function are used as if they were fputs(). No need to have it search for %'s if there aren't going to be any.

Cleaned up.


- Joshua


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


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/adfbb671/attachment-0001.htm>