[asterisk-dev] [Code Review] New application JabberReceive, implement SendText in chan_gtalk and chan_jingle

Philippe Sultan philippe.sultan at gmail.com
Wed Dec 24 10:19:19 CST 2008 


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > As a general comment, please update the CHANGES file with a description of the new features.

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/configs/jabber.conf.sample, line 23
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1945#file1945line23>
> >
> >     It's not clear to me what this option does.  Can you expand on the documentation?  Also, was it not in seconds before?  If not, then it is important to document this change in behavior in UPGRADE.txt.

This value represents the maximum number of seconds incoming messages are stored by the corresponding Asterisk's XMPP client. Here is how messages are processed :
1) if we don't receive any XMPP data for 2 seconds, all stored messages are deleted.
2) Upon XMPP message reception from a given JID, all messages stored more than 2 seconds from this JID are deleted, and the new message is stored.
3) acf_jabberreceive_read gets the first (most recent) message from the given JID, and returns its content to the dialplan.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 99
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line99>
> >
> >     Please trim the trailing whitespace before the final version of the diff.

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 554
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line554>
> >
> >     Document each parameter separately
> >     
> >     \param chan The associated ast_channel, if there is one
> >     \param data The account, JID, and optional timeout

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 556
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line556>
> >
> >     \retval 0 success
> >     \retval -1 failure

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 587
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line587>
> >
> >     Does this function return an ASTOBJ reference?
> >     
> >     If it does, then you have a reference leak in this function.
> >     
> >     If not, then it really, really, should, but we can address that separately, as it is not related to this patch.

Yes it does. Fixed in new diff. All the XMPP modules need to be checked though.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, lines 602-606
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line602>
> >
> >     I prefer using sscanf() for this error handling.

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 616
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line616>
> >
> >     This looks like a memory allocation.  If it is, you have a memory leak.

Yep, I completely changed the way we handle that, why? Because there is no corresponding freeing function in iksemel :)


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 646
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line646>
> >
> >     I see that you're using ast_safe_sleep() here to ensure that the channel is being serviced while this function blocks.  However, I would suggest that you use ast_autoservice_start()/stop() instead.  The reason is that autoservice does some additional handling to make sure that important signaling frames don't get dropped on the floor, and can be handled when the channel comes back out of autoservice.  With safe_sleep(), everything gets dropped.

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 668
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line668>
> >
> >     curly brace on the next line

ok.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, lines 684-702
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line684>
> >
> >     You have a memory leak here in that you're not freeing the aji_message struct itself

Yep, the new function aji_message_destroy will help us here.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 691
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line691>
> >
> >     remove space after deleted.

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 1734
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line1734>
> >
> >     This is not necessary.  ast_calloc() will generate an error log message for you.

ok.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c, line 1766
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1947#file1947line1766>
> >
> >     Check for allocation failure here

done.


> On 2008-12-18 08:11:17, Russell Bryant wrote:
> > http://svn.digium.com/svn/asterisk/trunk/channels/chan_gtalk.c, lines 1470-1481
> > <http://reviewboard.digium.com/r/88/diff/3/?file=1943#file1943line1470>
> >
> >     Is it possible for the contents of the gtalk_pvt to change out from under you during this function call?  Normally I would expect some sort of locking around the access to the pvt here.

No, we don't change the gtalk_pvt here. We just use a function from res_jabber.so to send an XMPP message.


- Philippe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/88/#review244
-----------------------------------------------------------


On 2008-12-18 04:35:30, Philippe Sultan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/88/
> -----------------------------------------------------------
> 
> (Updated 2008-12-18 04:35:30)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> JabberReceive is a dialplan application that makes Asterisk wait for an XMPP message from a given user (identified with his JID), and store the content in a dialplan variable.
> 
> It can be used for example to notify a user (via XMPP) that a call is coming, and read input back from him.
> Taken from the documentation :
> In the following example, calls targeted to extension 1234 (be it 
> accessed from SIP, DAHDI or whatever channel type) are controlled by
> user bob at jabber.org. Asterisk notifies him that a call is coming, and
> asks him to take an action. This dialog takes place over an XMPP chat.
> 
> context from-ext {
> 	1234 => {
> 		Answer();
> 		JabberSend(asterisk-xmpp,bob at jabber.org,Call from $CALLERID(num) - choose an option to process the call);
> 		JabberSend(asterisk-xmpp,bob at jabber.org,1 : forward to cellphone);
> 		JabberSend(asterisk-xmpp,bob at jabber.org,2 : forward to work phone);
> 		JabberSend(asterisk-xmpp,bob at jabber.org,Default action : forward to your voicemail);
> 		JabberReceive(bob at jabber.org,OPTION,20);
> 		switch (${OPTION}) {
> 			case 1:
> 	  			JabberSend(asterisk-xmpp,bob at jabber.org,(Calling cellphone...);
> 				Dial(SIP/987654321);
> 				break;
> 			case 2:
> 				JabberSend(asterisk-xmpp,bob at jabber.org,(Calling workphone...);
> 				Dial(SIP/${EXTEN});
> 				break;
> 			default:
> 				Voicemail(${EXTEN}|u)
> 		}
> 	}
> }
> 
> The diff also includes an implementation of SendText as XMPP messages in both chan_jingle and chan_gtalk.
> 
> The corresponding bug contains more use cases : http://bugs.digium.com/view.php?id=12569
> 
> 
> This addresses bug 12569.
>     http://bugs.digium.com/view.php?id=12569
> 
> 
> Diffs
> -----
> 
>   http://svn.digium.com/svn/asterisk/trunk/channels/chan_gtalk.c 165432 
>   http://svn.digium.com/svn/asterisk/trunk/channels/chan_jingle.c 165432 
>   http://svn.digium.com/svn/asterisk/trunk/configs/jabber.conf.sample 165432 
>   http://svn.digium.com/svn/asterisk/trunk/doc/jabber.txt 165432 
>   http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c 165432 
> 
> Diff: http://reviewboard.digium.com/r/88/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Philippe
> 
>

More information about the asterisk-dev mailing list