[asterisk-dev] [Code Review] Avoid infinite loop with certain local channel connected line updates

[Terry Wilson]

> On 2010-09-20 17:32:39, Russell Bryant wrote:
> > /branches/1.8/main/channel.c, lines 8191-8193
> > <https://reviewboard.asterisk.org/r/932/diff/1/?file=12580#file12580line8191>
> >
> >     Are you sure a string comparison is valid here?  Maybe a memcmp() would be safer?

Good call. Will change.


> On 2010-09-20 17:32:39, Russell Bryant wrote:
> > /branches/1.8/main/channel.c, lines 8203-8206
> > <https://reviewboard.asterisk.org/r/932/diff/1/?file=12580#file12580line8203>
> >
> >     I'm wondering if the fix should be localized to chan_local.  Could there be a legitimate case where we want to generate an update, even if it's already set to the same thing on the channel?  perhaps to get the information to a new downstream destination or something?

When I asked mmichelson that earlier he said, "I can't think of any. Sending connected line updates when there's actually been no change is a bad thing."


> On 2010-09-20 17:32:39, Russell Bryant wrote:
> > /branches/1.8/main/channel.c, lines 8181-8182
> > <https://reviewboard.asterisk.org/r/932/diff/1/?file=12580#file12580line8181>
> >
> >     spaces inside { }

ew, really? Ok. :-)


- Terry


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


On 2010-09-20 17:22:58, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/932/
> -----------------------------------------------------------
> 
> (Updated 2010-09-20 17:22:58)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> It is possible to create an infinite loop with connected line updates and local channels. One side of the local channel queues an update to the other side in local_indicate() which then causes the other side to do the same ad infinitum. This patch adds a function to compare the connected line update with the connected line info already on a channel. If there is no difference, it doesn't send the update frame.
> 
> It uses ast_connected_line_build_data since that function would need to be changed if any of the ast_connected_line-related structs changed and trying to compare each setting individually is really a pain and would be just another function to update.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/channel.c 287638 
> 
> Diff: https://reviewboard.asterisk.org/r/932/diff
> 
> 
> Testing
> -------
> 
> queues.conf
> ---------
> [general]
> persistentmembers = yes
> autofill = yes
> monitor-type = MixMonitor
> shared_lastcall=no
> 
> [test]
> strategy=ringall
> member => Local/6001 at sip/nm
> reportholdtime=yes
> 
> extensions.conf
> -------------
> [sip]
> exten => 123,1,NoOp
> exten => 123,n,Answer
> exten => 123,n,Queue(test)
> 
> exten => _XXXX,1,Set(CONNECTEDLINE(num,i)=5551212)
> exten => _XXXX,n,Set(CONNECTEDLINE(name,i)=Widgets)
> exten => _XXXX,n,Set(__CONNECTED_LINE_CALLEE_SEND_MACRO=CONNECTED_LINE_MANIPULATION)
> exten => _XXXX,n,Set(__CONNECTED_LINE_CALLER_SEND_MACRO=CONNECTED_LINE_MANIPULATION)
> exten => _XXXX,n,Dial(SIP/${EXTEN})
> 
> [macro-CONNECTED_LINE_MANIPULATION]
> exten => s,1,NoOp(MACRO WAS CALLED)
> 
> sip.conf
> ------
> 
> [test](!)
> type=friend
> host=dynamic
> secret=whatever
> canreinvite=no
> context=sip
> callgroup=1
> pickupgroup=1
> 
> 
> [6001](test)
> [6002](test)
> 
> To reproduce
> -----------
> 6001 calls 100, puts on hold and dials *8 to pickup own call and transfers the the original call (in Eyebeam dials 100 on line 1, dials *8 on line two, hit XFER and click on line 1).
> 
> Before the patch, the macro is executed indefinitely. After, only once.
> 
> 
> Thanks,
> 
> Terry
> 
>