[Home]

Summary:ASTERISK-12283: [patch] Convert chan_skinny's open-coded linked lists to the list macros
Reporter:dea (dea)Labels:
Date Opened:2008-06-30 10:51:56Date Closed:2008-07-13 17:52:09
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_skinny-linkedlists.txt
( 1) chan_skinny-linkedlists-v2.txt
Description:No new warnings or errors.  There are a number of quirks relating to
transfers and multiple calls on hold, but these exist in Trunk before
the patch.
Comments:By: Russell Bryant (russell) 2008-06-30 11:51:52

1) Looks like you may have introduced a memory leak here:

@@ -1890,6 +1892,10 @@
if (!(req = req_alloc(sizeof(struct call_info_message), CALL_INFO_MESSAGE)))
return;

+ /* We should not be able to get here without a session */
+ if (!s)
+ return;


2) AST_LIST_FIRST is not needed here

+ s = AST_LIST_FIRST(&sessions);
+ while((s = AST_LIST_REMOVE_HEAD(&sessions, list))) {
By: dea (dea) 2008-06-30 15:37:51

Version 2 addresses Russell's comments.

I found and fixed the issue with Transfers that was leaving the phone in an
inconsistent state, as well as using the 'New Call' button when an existing
call was on hold.  These changes could be applied seperately, and if the
linked-list conversion is not likely to make the 1.6 cut, I will provide
them in a seperate patch.

While the conversion/patch works, I am confused by AST_LIST_EMPTY and how
I ended up using it in skinny_hangup().  An empty list means the phone
only had one call associated with it and needs a different cleanup process
than of the phone had multiple calls.

The comments for AST_LIST_EMPTY returns 0 if the list is empty, but looking
at the macro definition and what I observed the code doing, the opposite
seems to be happening.
By: Michiel van Baak (mvanbaak) 2008-07-04 17:30:58

applies, compiles and runs fine.
This is a home setup with two phones and roughly 10 calls a day
It's been running nice for two days now
By: dea (dea) 2008-07-05 01:26:48

That is good to hear.  I have a test scenario fo you (it may not work out
in a home lab/setup)

1.  Place a call
2.  Put it on hold
3.  Place another call
4.  Put it on hold
5.  Place a third call
6.  Put it on hold
7.  Select the call from 3 and resume
8.  Hang it up
9.  Select the call from 1 or 5
10. Hang it up
11. Resume the final call and hang it up

What do you see?  I have found that I can start three calls and
hang them up in ascending or descendign order with out issue, but
if I hang up a call in the middle odd behaviour is the result.

I should note that the is occurs (or worse) before the link list conversion.
I'd like to fix it, yet I also doubt it will effect too many users...


By: Michiel van Baak (mvanbaak) 2008-07-05 05:02:22

does it matter what I call ?
If not, I can setup calls to another asterisk box and run echo or something there
By: Michiel van Baak (mvanbaak) 2008-07-05 05:32:15

"What do you see?"

Once I select the second call (in the middle that is) and resume everything works
If I hangup this call I see call 1 and call three, with a single line box in between. with 00 at the start of that line.
I can now hangup either call 1 or 3 without problem and the whole line dissappears from the display.
The singleline box either shows on top or on bottom of the one call left on hold.
I can select both, but can only resume/hangup the real call.

If all calls are hangup, there's still this oneline box with 'Connected' on the bottom of my screen.
I cannot hangup this one, or resume/hold or whatever.

'core show channels' show no channels active.
'core show locks' is empty
'core show hints' show all lines as Idle

Hope this helps a bit.
By: dea (dea) 2008-07-06 01:25:09

It does help.  It confirms the issue is not unique to my lab
setup.  I do not believe it is related to the linked-list
conversion, and should not block merging.

I will keep looking into it and open another bug once I have
an idea how I want to fix it.
By: Michiel van Baak (mvanbaak) 2008-07-13 16:49:57

I can confirm that it's not related to the linked-list stuff.
I tried the same on a clean trunk checkout and have the same issue.

If there are no objections, I like to merge this one into trunk.
By: Digium Subversion (svnbot) 2008-07-13 17:52:03

Repository: asterisk
Revision: 130576

U   trunk/channels/chan_skinny.c

------------------------------------------------------------------------
r130576 | mvanbaak | 2008-07-13 17:52:02 -0500 (Sun, 13 Jul 2008) | 9 lines

Convert chan_skinny's open-coded linked lists to the list macros

(closes issue ASTERISK-12283)
Reported by: DEA
Patches:
     chan_skinny-linkedlists-v2.txt uploaded by DEA (license 3)
Tested by: DEA, mvanbaak


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=130576