[asterisk-dev] [Code Review] New application JabberReceive, implement SendText in chan_gtalk and chan_jingle
Russell Bryant
russell at digium.com
Mon Apr 27 07:45:42 CDT 2009
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/88/#review749
-----------------------------------------------------------
http://svn.digium.com/svn/asterisk/trunk/CHANGES
<http://reviewboard.digium.com/r/88/#comment1890>
The changes to CHANGES will need to be moved into the 1.6.3 section.
http://svn.digium.com/svn/asterisk/trunk/channels/chan_gtalk.c
<http://reviewboard.digium.com/r/88/#comment1891>
The res variable is always zero here. Would it be more useful to return a status code from ast_aji_send_chat() ?
http://svn.digium.com/svn/asterisk/trunk/channels/chan_jingle.c
<http://reviewboard.digium.com/r/88/#comment1892>
I have the same comment about res here.
http://svn.digium.com/svn/asterisk/trunk/doc/jabber.txt
<http://reviewboard.digium.com/r/88/#comment1893>
I believe you are the only current maintainer.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1894>
You have trailing whitespace throughout the patch that reviewboard highlights for you. Be sure to trim it before commit.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1895>
There is an extra space after read
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1896>
There is some inconsistent indentation here
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1897>
There are a number of places throughout the rest of this function where the channel is not taken back out of autoservice when returning due to an error.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1900>
Remove space after resource
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1898>
You call strlen(jid.screenname) and strlen(jid.resource) a couple of times in this loop. It will be more efficient to calculate it once outside of these loops and just used the saved off result.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1901>
The way this is written, it will check for a message, and if it doesn't find one, sleep for half a second before looking again.
Instead, the most elegant way to handle this would be to use a pthread lock and condition in combination with ast_pthread_cond_timedwait() and have the code receiving jabber messages wake up the thread if a matching message arrives before the timeout. That way, you don't needlessly loop through the messages multiple times while waiting, and you also will be able to continue immediately once the appropriate message arrives.
Let me know if you have any questions about my suggestion here.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1899>
Add a space after if
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1903>
The exact same thing will happen if you remove this check completely.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1902>
Remove space after deleted.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1904>
Add the \internal tag to documentation of internal functions.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment1905>
While you're changing formatting of this line anyway, there should be a space after if.
- Russell
On 2009-04-17 09:18:26, Philippe Sultan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/88/
> -----------------------------------------------------------
>
> (Updated 2009-04-17 09:18:26)
>
>
> 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/CHANGES 188900
> http://svn.digium.com/svn/asterisk/trunk/channels/chan_gtalk.c 188900
> http://svn.digium.com/svn/asterisk/trunk/channels/chan_jingle.c 188900
> http://svn.digium.com/svn/asterisk/trunk/configs/jabber.conf.sample 188900
> http://svn.digium.com/svn/asterisk/trunk/doc/jabber.txt 188900
> http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c 188900
>
> Diff: http://reviewboard.digium.com/r/88/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Philippe
>
>
More information about the asterisk-dev
mailing list