[Home]

Summary:ASTERISK-13000: [patch] segfault checking messages using mwi events
Reporter:dea (dea)Labels:
Date Opened:2008-10-31 23:01:38Date Closed:2009-01-19 14:09:01.000-0600
Priority:CriticalRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 2009011900_13821_skinnymwi.diff.txt
( 1) chan_skinny-mwi-events.txt
( 2) skinny_mwicrash.diff.txt
Description:I have been chasing a regular crash involves chan_skinny and IMAP enable
voicemail.

I'm following the bugs related to the c-client related crashes, which may
help, but the more I looked at the code that I ported from chan_zap to
chan_skinny to enable mwi events, the more convinced I became that it was just
not right.

So I dove into chan_sip to get a handle on how mwi event callbacks should be
used and developed the attached patch.

****** ADDITIONAL INFORMATION ******

I am marking this as 1.6.0.1, since the crash does occur there, but it effects trunk and 1.6.1 as well
Comments:By: Leif Madsen (lmadsen) 2008-12-22 11:30:30.000-0600

Since mvanbaak has said he is willing to have chan_skinny issues assigned to him, I'm reassigning this issue to him in the hopes we can move this forward. Thanks for the patch!
By: Michiel van Baak (mvanbaak) 2008-12-25 16:43:25.000-0600

I'm not sure wether the SKINNY_LAMP_BLINK is a toggle.
Since you are sending this to the phone when the mwiblink is set, no matter if there's voicemail waiting or not.
I think you just have to send SKINNY_LAMP_OFF in that case instead of the _BLINK

Other than that the patch looks fine and I'll testrun it once I get home this weekend.
By: dea (dea) 2008-12-28 15:16:12.000-0600

The lines with SKINHY_LAMP_BLINK control the icon next to the line
buttons (and the back-light buttons on the newer phones).
I did not trace out those calls or do a packet capture, I just brought
forward the code that was already there.

Let me know if it needs further rework.
By: Michiel van Baak (mvanbaak) 2009-01-16 11:28:09.000-0600

new patch uploaded that applies to trunk.
I only test-compiled it, no running done yet.

I'll ask wedhorn to have a look because of the whole line/device rewrite he did.
By: Michiel van Baak (mvanbaak) 2009-01-18 05:26:26.000-0600

+ if (new_msgs) {
+ transmit_lamp_indication(d, STIMULUS_VOICEMAIL, l->instance, l->mwiblink?SKINNY_LAMP_BLINK:SKINNY_LAMP_ON);
+ transmit_lamp_indication(d, STIMULUS_VOICEMAIL, 0, SKINNY_LAMP_ON);
+ } else {
+ transmit_lamp_indication(d, STIMULUS_VOICEMAIL, l->instance, SKINNY_LAMP_OFF);
+ transmit_lamp_indication(d, STIMULUS_VOICEMAIL, 0, SKINNY_LAMP_OFF);
+ }

That's how I think it should be.

Besides that wedhord brought up a good issue:
If a device has more then one line, the device MWI indicator will be wrong. We have to set the WMI for the line here, and then walk the lines attached to the same device to see if they have voicemail. That way the device mwi will be on when there's one or more lines on the device with voicemail, and off if no line attached to it has vm.

[edited because I pasted the if statement on top twice by accident]


By: Michiel van Baak (mvanbaak) 2009-01-19 12:31:33.000-0600

uploaded new patch that fixes the mwiblink de-activation.
Also put the code to control the device lamp in that patch. Going to ask wedhorn for a review
By: Digium Subversion (svnbot) 2009-01-19 14:08:59.000-0600

Repository: asterisk
Revision: 169367

U   trunk/channels/chan_skinny.c

------------------------------------------------------------------------
r169367 | mvanbaak | 2009-01-19 14:08:59 -0600 (Mon, 19 Jan 2009) | 15 lines

Redo the event-based MWI in chan_skinny.

Dan saw regular segfaults with the old implementation and
rewrote it to make it really eventbased.
I altered it to be trunk compatible and wedhorn gave some feedback
and ideas how to make it even better.

(closes issue ASTERISK-13000)
Reported by: DEA
Patches:
     chan_skinny-mwi-events.txt uploaded by DEA (license 3)
Tested by: mvanbaak, DEA

"no probs by me" from wedhorn

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

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