[asterisk-dev] [Code Review]: chan_jingle2: New Jingle + Google Talk channel driver
Joshua Colp
reviewboard at asterisk.org
Fri May 18 18:45:33 CDT 2012
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 74-81
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line74>
> >
> > 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.
Removed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 161
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line161>
> >
> > If this is an ao2 object and uses Terry's new configuration work, this may not be needed.
Agreed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 227
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line227>
> >
> > Shouldn't we have a handler for write_text?
write_text is used for T140 text, which is unsupported in this channel driver.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 90
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line90>
> >
> > Is there a particular reason for this size? If the size is not limited, why not use a string field?
Changed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 167
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line167>
> >
> > 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.
It's not just used for removing the session. Without keeping the endpoint around as I have then incoming traffic for the session will never be handled (it's done on an endpoint basis, not a session basis). Believe me - I tried to find a way to get it done on a session basis...
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 175-177
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line175>
> >
> > Magic numbers. Either these should be string fields or at least have a #define.
Changed to string fields.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 187
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line187>
> >
> > 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.
> >
> >
This is *not* backwards compatible with the old channel driver. Naming it Jingle would break installations that are upgrading really really really quickly.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 1392
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1392>
> >
> > Does jingle have a concept of connected line? If so, it'd be nice to have this.
It does not.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1478-1479
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1478>
> >
> > These actions should probably be done in the destructor of the session.
Can't be, the endpoint has a reference to the session and for reasons I previously mentioned I can't change that.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 634
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line634>
> >
> > That's an odd size for a name
This is actually to work around bugs in other ICE implementations. You *should* be able to use a string with characters, but some of the ones I found during my testing require this to be numerical - so I just hash what our ICE implementation gives us (which right now contains characters, naturally).
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 567
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line567>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 588
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line588>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 623
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line623>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 638
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line638>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 698
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line698>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 704
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line704>
> >
> > There should be some log statement if this occurs
I disagree. RTCP support is setup on RTP always because we don't know if it will or will not be used, as a result this generates RTCP candidates. By adding a log statement in here every time this code is hit messages will be output about RTCP candidates.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 752-755
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line752>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 796-798
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line796>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 853-854
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line853>
> >
> > This should log an error.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1054-1056
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1054>
> >
> > Failures in protocol should be logged, at least as a debug message.
This is actually a ye olde allocation error, now being logged.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1077-1079
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1077>
> >
> > Failures in protocol should be logged, at least as a debug message.
Another memory allocation failure only.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 970-985
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line970>
> >
> > 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.
IMO No, which is why I made it a non-option. It doesn't hurt to try as this is the only method in Jingle to do DTMF. Changed to ensure it is always present.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1620-1623
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1620>
> >
> > 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.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1626-1629
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1626>
> >
> > 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.
It does. When the session is found in the action handler the reference count is increased, once the action handler is finished it gets bumped down.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1641-1644
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1641>
> >
> > Refactored this block of code into its own method.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1649-1651
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1649>
> >
> > 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.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1671-1672
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1671>
> >
> > This needs a logging message. Video negotiated but no video present should be logged.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1680-1681
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1680>
> >
> > 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.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1719-1720
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1719>
> >
> > 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.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1725-1727
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1725>
> >
> > Refactor this block of code into its own method.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1730-1731
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1730>
> >
> > 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.
There isn't really an error code for that I'm afraid, and AST_CAUSE_SWITCH_CONGESTION is not the same as congestion. It means something happened internally within the "switch" or in our case application that caused it to fail.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1804-1805
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1804>
> >
> > Refactor this block of code into its own method.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1807-1810
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1807>
> >
> > 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.
See previous comment.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 1925
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1925>
> >
> > 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.
Fixed it in the action hook instead.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2098-2103
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2098>
> >
> > If we get an action we don't understand, we should log something.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 2318
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2318>
> >
> > If this failed, it should probably also deregister the RTP glue and the channel technology and stop the scheduler thread.
Fixed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2079-2080
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2079>
> >
> > This sounds like something that should at least be a NOTICE
I've further documented why this happens, it also includes responses that have no response hook registered - so it would get very chatty if logged.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1939-1946
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1939>
> >
> > If we don't have a pbx thread, we should probably not create the channel.
Fixed up! We now properly tear things down in this situation.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 174
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line174>
> >
> > 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.
Now using the same lock_full strategy as chan_sip.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 822-826
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line822>
> >
> > 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.
Per my earlier comment this is now using the lock_full strategy that is present in chan_sip, but just like that... the locks are still locked. This is because while you can ensure the channel does not go away you can't ensure that you are performing actions on the *correct* channel without locking it so masquerading does not occur.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 832-837
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line832>
> >
> > 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)
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 1133
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1133>
> >
> > The channel does not need to be locked here. The channel ref count should be bumped through this section of code.
Fixed, but why should the channel ref be bumped? Nothing can interrupt up and cause this to go away...
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 1262
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1262>
> >
> > The channel should already be locked through here - the ref count should be bumped only.
See above.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 1607
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1607>
> >
> > 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.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1983-1988
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1983>
> >
> > 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.
Changed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 2006
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2006>
> >
> > 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.
Changed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2011-2013
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2011>
> >
> > 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.
Changed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2038-2040
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2038>
> >
> > Again, these operations should only require a bump on the channel reference count.
Changed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 2057
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2057>
> >
> > Again, these operations should only require a bump on the channel reference count.
Changed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1030-1056
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1030>
> >
> > Refactor this block of code into its own smaller method.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 1058-1080
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1058>
> >
> > Refactor this block of code into its own smaller method.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 1456
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line1456>
> >
> > 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.
> >
> >
I have now fixed this. The session is locked during this procedure and once unlocked anything blocking should gracefully handle the situation.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 96
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line96>
> >
> > This should probably be a configurable parameter, unless its driven by one of the standards.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 99
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line99>
> >
> > This should probably be a configurable parameter, unless its driven by one of the standards.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 119-132
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line119>
> >
> > 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.
The namespaces will never change, unless a new protocol comes into play. Making it configurable would not buy us anything, unless the code is so awesome that it could adapt to the new protocol without any changes (doubtful).
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2156-2168
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2156>
> >
> > These methods have things inside them that can fail and should report it as such.
store_config_general is now a placeholder in case general options need to be added in the future
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2204-2220
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2204>
> >
> > This should eventually be replaced with Terry's new config work.
Agreed.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2259-2265
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2259>
> >
> > 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.
Fixed up.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, line 2326
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2326>
> >
> > 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.
Done.
> On May 14, 2012, 11:57 a.m., Matt Jordan wrote:
> > /trunk/channels/chan_jingle2.c, lines 2178-2196
> > <https://reviewboard.asterisk.org/r/1917/diff/2/?file=27832#file27832line2178>
> >
> > 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.
Fixed the race condition. Ultimately yes this will be migrated to Terry's code and heck it may take long enough to get ICE/STUN/TURN in and then this to be able to do it before it is merged in. We'll see.
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1917/#review6207
-----------------------------------------------------------
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/20120518/6824814e/attachment-0001.htm>
More information about the asterisk-dev
mailing list