[asterisk-dev] [Code Review] 2493: Add WebSocket transport module

[Joshua Colp]

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



/team/group/pimp_my_sip/include/asterisk/res_sip.h
<https://reviewboard.asterisk.org/r/2493/#comment16538>

    This would be more efficient if NOT done in sorcery. This data isn't going to be persisted as it can't be, it stores a direct pointer to a data structure. Using this directly in an ao2 container also means you can make the container use an rwlock, instead of a mutex.



/team/group/pimp_my_sip/res/res_sip.c
<https://reviewboard.asterisk.org/r/2493/#comment16539>

    There was a reason for my madness here... hopefully this still works in whatever case it occurred.



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16540>

    I defer to you as you know this API, is it safe for nothing to be done here?



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16541>

    Pool could leak here.



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16542>

    You need to allocate rdata *once* or else the memory pool will grow and grow and grow. Once pjsip_tpmgr_receive_packet returns it is safe to use it again.



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16543>

    This won't exceed the available space in rdata will it?



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16545>

    Why do you create the transport now and not when the connection starts?



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16534>

    This now introduces more of a bottle neck on outgoing requests and I think using the URI isn't safe (since it's possible an implementation may use the same contact across multiple instances, even though it probably shouldn't).
    
    Doesn't pjsip have a mechanism to do resolution to active transports based on the URI already? Is it possible to leverage that and add a parameter to the URI to make it unique?
    
    Regardless to make this more lightweight you could examine the transport on the URI and see if it is for websocket, if not then don't even do a lookup.



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16535>

    Make this more specific, or else a compiler might whine.



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16537>

    Same here, may fail.



/team/group/pimp_my_sip/res/res_sip_transport_websocket.c
<https://reviewboard.asterisk.org/r/2493/#comment16536>

    It's possible for this to return NULL.


- Joshua Colp


On May 8, 2013, 4:13 p.m., Jason Parker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2493/
> -----------------------------------------------------------
> 
> (Updated May 8, 2013, 4:13 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-20952
>     https://issues.asterisk.org/jira/browse/ASTERISK-20952
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds a custom WebSocket transport module.
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/include/asterisk/res_sip.h 387967 
>   /team/group/pimp_my_sip/res/res_sip.c 387967 
>   /team/group/pimp_my_sip/res/res_sip.exports.in 387967 
>   /team/group/pimp_my_sip/res/res_sip/config_transport.c 387967 
>   /team/group/pimp_my_sip/res/res_sip/location.c 387967 
>   /team/group/pimp_my_sip/res/res_sip_outbound_registration.c 387967 
>   /team/group/pimp_my_sip/res/res_sip_transport_websocket.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2493/diff/
> 
> 
> Testing
> -------
> 
> Registration works, incoming (from the browser) and outgoing (to the browser) calls work, audio flows (though, I don't have a mic, so I didn't test audio that direction).
> 
> 
> Thanks,
> 
> Jason Parker
> 
>

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