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

[wdoekes]

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



/trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/1952/#comment11835>

    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.



/trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/1952/#comment11836>

    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.



/trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/1952/#comment11837>

    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.


- wdoekes


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