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

[Jason Parker]

> On May 9, 2013, 5:12 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_transport_websocket.c, lines 252-262
> > <https://reviewboard.asterisk.org/r/2493/diff/2/?file=37424#file37424line252>
> >
> >     Why do you create the transport now and not when the connection starts?

There was some reason I moved it, but don't remember now.  I moved it out of the loop though.


> On May 9, 2013, 5:12 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_transport_websocket.c, lines 73-76
> > <https://reviewboard.asterisk.org/r/2493/diff/2/?file=37424#file37424line73>
> >
> >     I defer to you as you know this API, is it safe for nothing to be done here?

It's fine, but the pointer doesn't even actually need to be set, so I just removed the function.


> On May 9, 2013, 5:12 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_transport_websocket.c, line 319
> > <https://reviewboard.asterisk.org/r/2493/diff/2/?file=37424#file37424line319>
> >
> >     Make this more specific, or else a compiler might whine.

/me adds long to the list of possible types a transport_type can be represented as in pjsip.


> On May 9, 2013, 5:12 p.m., Joshua Colp wrote:
> > /team/group/pimp_my_sip/res/res_sip_transport_websocket.c, line 291
> > <https://reviewboard.asterisk.org/r/2493/diff/2/?file=37424#file37424line291>
> >
> >     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.

Finding a transport requires DNS resolution.  We can't let it try to do that, because of the .invalid TLD that gets used.

I'm not finishing this thought right now, since reviewboard currently hates me.


- Jason


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


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