[asterisk-dev] [Code Review] New application JabberReceive, implement SendText in chan_gtalk and chan_jingle
Russell Bryant
russell at digium.com
Thu Dec 18 08:11:16 CST 2008
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/88/#review244
-----------------------------------------------------------
As a general comment, please update the CHANGES file with a description of the new features.
http://svn.digium.com/svn/asterisk/trunk/channels/chan_gtalk.c
<http://reviewboard.digium.com/r/88/#comment486>
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.
http://svn.digium.com/svn/asterisk/trunk/channels/chan_jingle.c
<http://reviewboard.digium.com/r/88/#comment487>
My same comments about locking apply to this code as well.
http://svn.digium.com/svn/asterisk/trunk/configs/jabber.conf.sample
<http://reviewboard.digium.com/r/88/#comment488>
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.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment489>
Please trim the trailing whitespace before the final version of the diff.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment491>
Document each parameter separately
\param chan The associated ast_channel, if there is one
\param data The account, JID, and optional timeout
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment490>
\retval 0 success
\retval -1 failure
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment492>
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.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment493>
I prefer using sscanf() for this error handling.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment494>
This looks like a memory allocation. If it is, you have a memory leak.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment495>
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.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment496>
curly brace on the next line
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment498>
You have a memory leak here in that you're not freeing the aji_message struct itself
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment497>
remove space after deleted.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment499>
This is not necessary. ast_calloc() will generate an error log message for you.
http://svn.digium.com/svn/asterisk/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/88/#comment500>
Check for allocation failure here
- Russell
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