[asterisk-dev] [Code Review]: Update a peer's lastmsgssent value appropriately

[Matt Jordan]

> On May 22, 2012, 3:30 p.m., Mark Michelson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 25688-25693
> > <https://reviewboard.asterisk.org/r/1939/diff/1/?file=28178#file28178line25688>
> >
> >     You may as well just wait to unlock the peer after the call to update_peer_lastmsgsent() and just pass 1 as the final parameter.

I thought about that, but on the nominal path of execution the peer still needs to be unlocked.  If I choose not to unlock before checking if the mailbox is an empty string, I'm holding the peer lock longer then necessary and I'd still have to unlock it if the mailbox is not an empty string.  As it is, I'll leave these two as they currently are.


- Matt


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


On May 22, 2012, 12:22 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1939/
> -----------------------------------------------------------
> 
> (Updated May 22, 2012, 12:22 p.m.)
> 
> 
> Review request for Asterisk Developers and irroot.
> 
> 
> Summary
> -------
> 
> In prior versions of Asterisk, the lastmsgssent value was used to track whether or not a peer had received MWI notifications.  Since chan_sip polled for the notifications itself, the value was rather important.  When MWI notifications were changed to use the event notification framework, the value was no longer useful for anything other then reporting through the CLI or AMI events.
> 
> Hence, in Asterisk 10 and trunk, the value was completely removed.  Unfortunately, in Asterisk 1.8, the value was not removed; instead, it is set to a value of -1 and never updated.  Since the lower 16 bits are used for old messages and the upper 16 bits are used for new messages, this results in the following being displayed for 'sip show peer foo':
> 
> LastMsgsSent : 32767/65535
> 
> Normally, I'd suggest that we remove the field from Asterisk 1.8 and call it a day.  However, since this field was supplied to users via AMI and the CLI, doing so breaks backwards compatibility.
> 
> This patch is a modification of a patch originally supplied by irroot on ASTERIS-17866.  It re-implements updating of lastmsgssent when an MWI notification is sent to a SIP peer.  Note that the original patch had to be modified slightly due to changes in sip_send_mwi_to_peer (and that up to three threads can be involved in accessing lastmsgssent means that, at the very least, the peer really should be locked when we update it).
> 
> If we decide that this isn't worth it, we should instead remove the field from Asterisk 1.8, as reporting an erroneous value isn't terribly useful.
> 
> 
> This addresses bug ASTERISK-17866.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17866
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 367134 
> 
> Diff: https://reviewboard.asterisk.org/r/1939/diff
> 
> 
> Testing
> -------
> 
> Tested with two SIP realtime peers.  One peer without voicemail continued to display the 32767/65535 value - which is expected, as that value indicates that we haven't sent any message notifications.  Another peer, with voicemail, correctly showed the new/old message counts post registration, and displayed the counts correctly after MWI notifications were sent when new voicemails were left or listened to.
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120523/d3d5e1d3/attachment.htm>