[asterisk-dev] [Code Review] 3541: res_http_websocket: Create a websocket client

[wdoekes]

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

Ship it!


Looks mostly good. A few minor points below.


trunk/include/asterisk/uri.h
<https://reviewboard.asterisk.org/r/3541/#comment21926>

    host name *or*, right?
    
    Looks like a mistake by a Dutchman, where 'of' means 'or' ;-)



trunk/include/asterisk/uri.h
<https://reviewboard.asterisk.org/r/3541/#comment21927>

    Same as above.



trunk/include/asterisk/uri.h
<https://reviewboard.asterisk.org/r/3541/#comment21936>

    I'd call this "ast_uri_make_host_with_port" so it's clear that we're returning a new string that has to be freed, in constrast to "ast_uri_host" and friends.



trunk/main/http.c
<https://reviewboard.asterisk.org/r/3541/#comment21937>

    Do we have any assurances that strlen(s) is small-ish (<8192)?
    
    Stack size is small in asterisk and we don't want a large header to smash it.
    
    (The other call to ast_alloca below is safe, since we check it against max 8192.)
    
    
    Secondly, you're missing a +1. Take s="a", now *buf_end='\0' writes past the room.



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21931>

    A short comment above this to the effect of: 
    
    the size parameters are including the trailing the NUL, e.g. you pass "http:...", 5 to get "http" in the scheme.
    



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21939>

    I would've favored a linefeed before ast_uri_is_secure, so the ternary goes on a single line.



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21933>

    If prefer the array-index syntax:
    
      res[host_size] = ':';
    
    (For positive indexes. For negative ones like  *(uri - 1) I like the pointer arithmetic instead. Not sure why.)



trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/3541/#comment21935>

    ast_strdup



trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/3541/#comment21938>

    Linefeed.



trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/3541/#comment21940>

    I believe coding guidelines says we should split these over multiple lines. But I don't mind.


- wdoekes


On May 29, 2014, 6:07 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3541/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 6:07 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Bugs: ASTERISK-23742
>     https://issues.asterisk.org/jira/browse/ASTERISK-23742
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Add client websocket capabilities to Asterisk.
> 
> 
> Diffs
> -----
> 
>   trunk/tests/test_websocket_client.c PRE-CREATION 
>   trunk/res/res_http_websocket.exports.in 414813 
>   trunk/res/res_http_websocket.c 414813 
>   trunk/main/uri.c PRE-CREATION 
>   trunk/main/http.c 414813 
>   trunk/include/asterisk/uri.h PRE-CREATION 
>   trunk/include/asterisk/http_websocket.h 414813 
>   trunk/include/asterisk/http.h 414813 
> 
> Diff: https://reviewboard.asterisk.org/r/3541/diff/
> 
> 
> Testing
> -------
> 
> Created some unit tests.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140529/b899691d/attachment-0001.html>