[asterisk-dev] [Code Review]: res_xmpp - A cleaned up drop-in replacement for res_jabber

Thu Jun 21 08:15:58 CDT 2012

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 445-453
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line445>
> >
> >     You may be risking a double free here. ast_xmpp_client_disconnect() will call iks_parser_delete() but it does not then set client->parser to NULL after. The result is that iks_parser_delete() will be called a second time here.
> >     
> >     On the other hand, iks_filter_delete() is called in ast_xmpp_client_disconnect() but then client->filter is set NULL, so iks_filter_delete() will not be called a second time.
> >     
> >     You'd probably do well to just remove these function calls from xmpp_client_destructor() entirely.

Done.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 744-749
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line744>
> >
> >     xmpp_pubsub_iq_create() can return NULL. Is it safe to pass a NULL pointer as the first parameter to iks_insert()?
> >     
> >     EDIT: I've seen that passing NULL to iks_insert is fine, but some of these also end up calling ast_xmpp_client_send(). This appears not to be safe for sending a NULL parameter.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 964-967
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line964>
> >
> >     Another case where the result of xmpp_pubsub_iq_create() is not checked for NULL.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 1090-1092
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line1090>
> >
> >     Another missing NULL return check.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 2737-2742
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line2737>
> >
> >     The unlock here looks wrong. Where was client->buddies locked previously?

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 2875-2888
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line2875>
> >
> >     Seems weird to keep the buddies container locked like this. ao2_find can lock the container when searching for the buddy and then ao2_link will lock the container in xmpp_client_create_buddy().

It's to ensure that the roster does not suddenly have the buddy in it by means of reconfiguration.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 2917-2920
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line2917>
> >
> >     The comment says "semicolon" but the code is checking for a colon. Which is it supposed to be?

This is legacy code that I didn't want to touch for fear of breaking reality in weird circumstances. I've changed the comment to reflect what it actually does.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 2730
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line2730>
> >
> >     6 is an odd thing to have here. Use a constant.

Changed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 3153
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3153>
> >
> >     Why is this a warning?

Changed to debug.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 3091
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3091>
> >
> >     Odd spacing here.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3086-3088
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3086>
> >
> >     Would ast_skip_blanks() work here?

It *could* but as that returns a pointer and not the position it wouldn't change what is done much... but I'm going to ponder a better way to do this.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 3464
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3464>
> >
> >     The "&& message" part of this if seems unnecessary since you've already bailed out earlier in the function if message is NULL.

Changed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3465-3469
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3465>
> >
> >     Shouldn't you check the return value of ast_xmpp_client_send_message() to determine if you send a Success or Error response?

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3490-3492
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3490>
> >
> >     Another missing NULL return check.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3538-3539
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3538>
> >
> >     xmpp_pubsub_build_node_request() can return NULL. Is it safe to continue without checking for a NULL return?

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 3626
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3626>
> >
> >     Check for NULL return of xmpp_publish_build_node_request().

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3848-3856
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3848>
> >
> >     The way this CLI command works is a bit unintuitive.
> >     
> >     The reason why is that it is changing a client value that is also affected by configuration.
> >     
> >     Say for instance that someone issues the "jabber set debug on" command. Then afterwards, he makes a small change to xmpp.conf and issues a reload. Any client that does not have "debug = yes" set in xmpp.conf will now no longer be debugged.
> >     
> >     My suggested fix for this is to have the CLI command set a file-scoped variable instead of setting XMPP_DEBUG on the individual clients. This way, if either XMPP_DEBUG is set on a client or if the file-scoped variable is set, then you would debug.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 3973
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3973>
> >
> >     This should be changed to "Google Talk capable"

Changed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 4058
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line4058>
> >
> >     Nitpick: Use AST_MODULE_LOAD_SUCCESS here.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 1599-1606
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line1599>
> >
> >     So depending on how the JabberReceive application is supposed to work, you may have a race condition here.
> >     
> >     It appears that the intent is to find messages in the queue that are for a specific client and within a specific timeframe. This means that messages that arrive before JabberReceive or during the setup portion at the beginning of JabberReceive should be detected.
> >     
> >     The problem here is that you're doing an ast_cond_timedwait() without first checking if the message queue has items in it. I would think you would only do the timedwait in the scenario where there are no messages already in the queue.

Fixed.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3280-3289
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3280>
> >
> >     I'm not sure that all of these apply (like client->state or client->status), but a lot of these default value settings should also be done in the case where the client is found in the container as well. The idea is that the client should have a default value set for an option in the case that the option has been removed from the config entirely.

Fixed with new configuration API.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3342-3343
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3342>
> >
> >     Is a NULL return here worthy of abandoning configuration of this client?

Now logging a message.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3351-3353
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3351>
> >
> >     Is a NULL return here worthy of abandoning configuration for this client?

Now logging a message.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3401-3405
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3401>
> >
> >     These error messages should either both mention both configuration files or else both only mention xmpp.conf

Fixed with new configuration API.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, line 3410
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3410>
> >
> >     You've got this combination of flags listed here twice when you really don't need to. Put these flags in a file-scoped variable and assign generalflags that variable's value on a configuration load.

Fixed with new configuration API.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3414-3420
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3414>
> >
> >     Since client flags depend on flags from the general section, you should change this to process the general section first and then each client.

Fixed with new configuration API.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 3648-3658
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line3648>
> >
> >     CLI commands should allow for "xmpp" in addition to "jabber" as the first word. In addition, xmpp.conf should be mentioned either alongside of or in place of jabber.conf.

Changed, and added aliases.

> On June 11, 2012, 12:54 p.m., Mark Michelson wrote:
> > /trunk/res/res_xmpp.c, lines 4060-4063
> > <https://reviewboard.asterisk.org/r/1983/diff/2/?file=28838#file28838line4060>
> >
> >     Hm, so the only condition under which this can be reached is if xmpp config couldn't be loaded. Do you think that merits AST_MODULE_LOAD_FAILURE or would AST_MODULE_LOAD_DECLINE fit here better?

Fixed up with configuration API changes.

- Joshua

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

On June 8, 2012, 4:12 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1983/
> -----------------------------------------------------------
> 
> (Updated June 8, 2012, 4:12 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is a cleaned up drop-in replacement for res_jabber. It provides the same functionality but internally has different API calls. It also uses astobj2 for most things.
> 
> As things stand right now this is using the old configuration API but this will be migrated to the new one once some additional things have been added. Existing modules have *not* been converted over, but chan_jingle2 will be.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/xmpp.h PRE-CREATION 
>   /trunk/res/res_xmpp.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1983/diff
> 
> 
> Testing
> -------
> 
> Tested client connections, component connections, authentication, roster adding/removing, all the options, pubsub, CLI commands, manager commands.
> 
> 
> Thanks,
> 
> Joshua
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120621/99bf72af/attachment-0001.htm>