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

[Kevin Harwell]

> On May 29, 2014, 8:07 a.m., wdoekes wrote:
> > trunk/include/asterisk/http.h, lines 271-272
> > <https://reviewboard.asterisk.org/r/3541/diff/3/?file=58979#file58979line271>
> >
> >     This is case sensitive,
> >     
> >     while ast_http_header_match is sensitive.
> >     
> >     I don't see that explained here.

I'll add a note and ensure both use case insensitive checks (per your other comment).


> On May 29, 2014, 8:07 a.m., wdoekes wrote:
> > trunk/main/http.c, lines 1199-1203
> > <https://reviewboard.asterisk.org/r/3541/diff/3/?file=58982#file58982line1199>
> >
> >     Why are you checking the reason phrase? Is that really necessary?
> >     
> >     
> >     6.1.1 Status Code and Reason Phrase
> >     
> >        The Status-Code element is a 3-digit integer result code of the
> >        attempt to understand and satisfy the request. These codes are fully
> >        defined in section 10. The Reason-Phrase is intended to give a short
> >        textual description of the Status-Code. The Status-Code is intended
> >        for use by automata and the Reason-Phrase is intended for the human
> >        user. The client is not required to examine or display the Reason-
> >        Phrase.

It is not necessary.  I thought it was required to check it for websockets (which would mean at the very least it needs to be optional or checked only for websockets), but after double checking the websocket rfc it is not required for those as well, so I'll remove the check.


> On May 29, 2014, 8:07 a.m., wdoekes wrote:
> > trunk/main/uri.c, lines 56-59
> > <https://reviewboard.asterisk.org/r/3541/diff/3/?file=58983#file58983line56>
> >
> >     If !param, shouldn't field be set to NULL? Or does ao2_alloc zero everything?

[looks at code] It looks like ao2_alloc uses ast_calloc to create the object so should be okay.


- Kevin


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


On May 28, 2014, 5:58 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3541/
> -----------------------------------------------------------
> 
> (Updated May 28, 2014, 5:58 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 414797 
>   trunk/res/res_http_websocket.c 414797 
>   trunk/main/uri.c PRE-CREATION 
>   trunk/main/http.c 414797 
>   trunk/include/asterisk/uri.h PRE-CREATION 
>   trunk/include/asterisk/http_websocket.h 414797 
>   trunk/include/asterisk/http.h 414797 
> 
> 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/866b5998/attachment-0001.html>