[asterisk-dev] [Code Review] chan_jingle2: New Jingle + Google Talk channel driver

Mon May 14 11:57:00 CDT 2012

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


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11372>

    I'm not sure we should provide a jitter buffer at the channel driver level.  Since people have the option of placing jitter buffers on the read side of the channel using JITTERBUFFER, giving them the option through the channel driver seems to provide them a less flexible, and less preferred, mechanism of doing something they already can do.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11373>

    Is there a particular reason for this size?  If the size is not limited, why not use a string field?


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11374>

    This should probably be a configurable parameter, unless its driven by one of the standards.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11375>

    This should probably be a configurable parameter, unless its driven by one of the standards.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11376>

    Do we want to go ahead and make the namespaces configurable?  It seems as if this is one of those fields that could be 'tweaked' by Google in the future, breaking compatibility.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11377>

    If this is an ao2 object and uses Terry's new configuration work, this may not be needed.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11380>

    The session should not have a pointer back to the endpoint, as it implies a strong association from the session to the endpoint that doesn't exist.  The danger here is that a circular dependency makes the locking order and the ownership semantics difficult to enforce.
    
    As this is only used during a hangup to notify the endpoint that one of its session objects has gone away, why not just cache the endpoint name?  Then you can safely do a lookup and ask the endpoint to remove the session.  This creates a much weaker reference between the session and the endpoint, and if the endpoint goes away during a hangup (such as during a CLI reload), there's less room for error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11400>

    The locking order between the owning channel and the session is not clearly defined throughout this channel driver.  In general, I'd expect the channel to always be locked first, followed by the session object.  If the session object is locked and need to access the channel - either by bumping its ref count or by actually obtaining a lock - there should be a consistent mechanism that does this that preserves the locking order and does not rely on deadlock prevention or result in double locks (just because we use recursive mutexes doesn't mean we should rely on them when we don't have to).  More on this later.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11378>

    Magic numbers.  Either these should be string fields or at least have a #define.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11379>

    I'm not sure we should have two extended support channel drivers.  If this is to replace the current chan_jingle, then I'd propose we name it Jingle, and the current chan_jingle would be renamed to chan_jingle_deprecated, or something like that.  The old channel driver could be named something to indicate its deprecated status and kept in case there is some element of backwards compatibility we missed, but I'd prefer people to just use the improved channel driver.
    
    
/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11429>

    Shouldn't we have a handler for write_text?


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11444>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11441>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11442>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11440>

    That's an odd size for a name


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11443>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11438>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11439>

    There should be some log statement if this occurs


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11437>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11436>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11399>

    I don't think we should be using ast_channel_trylock on the session owner.  This appears to be a workaround to the locking order difficulties that exist between a channel and it's pvt.
    
    In general, this should probably follow the same mechanism as sip_pvt_lock_full.  This would use something like the following:
    
    while (attempt_again)
      lock session
      set candidate_channel = session->owner
      if candidate_channel
         ref candidate_channel
      else
         unlock session and return
      unlock session
    
      lock candidate_channel
      lock session
      if (candidate_channel != session_owner)
        unlock everything and retry
      else
        do not attempt again
    
    At the end of this, the channel reference has been bumped one, and the channel and the session have been locked in the correct order.
    
    In some cases in chan_jingle2, we only need to ensure that the channel object does not get removed while we're performing some operation.  In those cases, a different method could be written similar to this that stops once the channel ref count has been bumped.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11415>

    Again, backwards, and again, we only need a ref on the channel, not a lock.
    
    This should probably at least have a debug log in here so that we know when we're hanging something up (usually useful)


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11435>

    This should log an error.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11434>

    Should there ever be a situation in which we do not negotiate DTMF?  It seems like the max audio formats allowed should be MAX_PAYLOADS - 2 in order to ensure that DTMF is always included in the negotiation.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11432>

    Refactor this block of code into its own smaller method.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11427>

    Failures in protocol should be logged, at least as a debug message.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11433>

    Refactor this block of code into its own smaller method.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11426>

    Failures in protocol should be logged, at least as a debug message.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11431>

    The channel does not need to be locked here.  The channel ref count should be bumped through this section of code.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11430>

    The channel should already be locked through here - the ref count should be bumped only.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11428>

    Does jingle have a concept of connected line?  If so, it'd be nice to have this.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11425>

    This method shows why the locking order needs to be channel then session.  In this method, while the channel is locked, we at the very least partially destroy the session.  We do this in situations where the session would be locked (implying it shouldn't change), but the channel has not yet been locked.
    
    In the current code, the order is often reversed and the session is locked and we do what amounts to deadlock prevention to gain a lock on the channel.  In that situation, the session could be locked while this method is executed.  In this method we would have the channel lock and, at the very least, partially destroy the session object.  When we unlock the channel, the session object has been removed from the endpoint and the channel, but the thread that was in the deadlock prevention believes that it has obtained a valid channel associated with the session.
    
    
/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11447>

    These actions should probably be done in the destructor of the session.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11387>

    This method should be refactored such that it calls smaller methods to do the logically distinct pieces of work.
    
    It should also document that the session/channel should not be locked prior to going into this method.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11411>

    Things that cause us to hangup due to errors in protocol should always be logged.  This is useful for when an endpoint sends us something Asterisk doesn't process, which typically end up in the issue tracker.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11413>

    It sort of feels like the session object, at the least, should have its ref bumped through all of this.  Otherwise the session could be disposed of while we still have a reference to the RTP object on the session.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11419>

    Refactored this block of code into its own method.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11414>

    Things that cause us to hangup due to errors in protocol should always be logged.  This is useful for when an endpoint sends us something Asterisk doesn't process, which typically end up in the issue tracker.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11416>

    This needs a logging message.  Video negotiated but no video present should be logged.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11417>

    Things that cause us to hangup due to errors in protocol should always be logged.  This is useful for when an endpoint sends us something Asterisk doesn't process, which typically end up in the issue tracker.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11421>

    Things that cause us to hangup due to errors in protocol should always be logged.  This is useful for when an endpoint sends us something Asterisk doesn't process, which typically end up in the issue tracker.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11420>

    Refactor this block of code into its own method.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11422>

    This needs to be logged.  Should this return congestion?  An endpoint requested ICE but the module isn't available in Asterisk - if there's an error code for 'server configuration error' or something along those lines that would be more appropriate.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11423>

    Refactor this block of code into its own method.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11424>

    This needs to be logged.  Should this return congestion?  An endpoint requested ICE but the module isn't available in Asterisk - if there's an error code for 'server configuration error' or something along those lines that would be more appropriate.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11445>

    This method should probably explicitly bump the ref count on the endpoint so that it lives through this entire operation.  Otherwise you could call into here and someone could perform a reload, causing the endpoint to be disposed of.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11410>

    If we don't have a pbx thread, we should probably not create the channel.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11407>

    This shouldn't need the channel lock.  It should instead have the ref count bumped on the channel until its does queueing the control frame.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11402>

    In all of these cases, we check if there's an owner after jingle_get_owner_lock.  It would seem that if a session does not have an owner, there's no need to attempt to get the owner lock.
    
    I'll grant that the current implementation of jingle_get_owner_lock handles this, but on a first pass through the code it makes the lock/unlock pair appear unbalanced.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11406>

    We only need to lock the channel around this section of code.  The rest of these operations only require that the channel reference be bumped.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11404>

    Again, these operations should only require a bump on the channel reference count.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11405>

    Again, these operations should only require a bump on the channel reference count.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11386>

    This sounds like something that should at least be a NOTICE


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11403>

    If we get an action we don't understand, we should log something.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11398>

    I know this is currently using ASTOBJ, but it shouldn't be.  Refactor the aji_client to be ast_aji_client (proper naming convention) and to be ao2 objects.
    
    While this affects the soon to be deprecated channel drivers, continuing dependencies on ASTOBJ is something I'd prefer to avoid.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11384>

    These methods have things inside them that can fail and should report it as such.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11385>

    There are race conditions in this code.  During a reload, if an endpoint is marked for deletion, nothing stops a new jingle channel being created for that endpoint.
    
    Ideally, during a reload there'd be a single point in time in which the endpoints are swapped out.  Channels that were created for a newly deleted endpoint would keep the old endpoint alive until the channel went away, at which point the endpoint object would be destroyed.
    
    Terry's new config work should make this type of operation possible without needing a destroy flag.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11397>

    This should eventually be replaced with Terry's new config work.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11383>

    These methods should be able to return failure.  If the configuration file cannot be loaded, and this is a reload operation, then the current configuration should not be changed.
    
    Again, Terry's config work should help with this.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11382>

    If this failed, it should probably also deregister the RTP glue and the channel technology and stop the scheduler thread.


/trunk/channels/chan_jingle2.c
<https://reviewboard.asterisk.org/r/1917/#comment11381>

    This needs to load the configs using the RELOAD flag.  That will make sure we don't reload config file options if the config file hasn't changed.


/trunk/include/asterisk/jingle.h
<https://reviewboard.asterisk.org/r/1917/#comment11446>

    Need to mark this file as deprecated if we're defining this all in the channel driver file.


- Matt


On May 13, 2012, 12:15 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1917/
> -----------------------------------------------------------
> 
> (Updated May 13, 2012, 12:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is a new channel driver written from scratch for the Jingle, Google Jingle, and Google Talk protocols. It has been written to the specs available and tested extensively.
> 
> ICE and STUN support for Jingle uses the new ICE/STUN/TURN support which is present in another review. (Please do not review any of that code in this review)
> STUN support for Google uses the existing STUN implementation, as the new support is not compatible with it.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_jingle2.c PRE-CREATION 
>   /trunk/channels/chan_sip.c 365451 
>   /trunk/configs/jingle2.conf.sample PRE-CREATION 
>   /trunk/configs/rtp.conf.sample 365451 
>   /trunk/include/asterisk/jabber.h 365451 
>   /trunk/include/asterisk/jingle.h 365451 
>   /trunk/include/asterisk/rtp_engine.h 365451 
>   /trunk/main/rtp_engine.c 365451 
>   /trunk/res/Makefile 365451 
>   /trunk/res/res_jabber.c 365451 
>   /trunk/res/res_rtp_asterisk.c 365451 
> 
> Diff: https://reviewboard.asterisk.org/r/1917/diff
> 
> 
> Testing
> -------
> 
> Tested audio calls with following:
> 
> GMail Google Talk Plug-in (and video)
> Google Voice
> Jitsi (and video)
> Psi
> OneTeam
> 
> * Included varying codecs (ulaw, speex, g722, etc)
> 
> Tested ringing, hold, and unhold with following:
> 
> Jitsi
> 
> Other clients do not support this.
> 
> 
> Thanks,
> 
> Joshua
> 
>

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