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

Mon Jun 11 12:54:46 CDT 2012

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

I didn't make it all the way through the request, and my eyes are starting to glaze over a bit so here's what I have so far.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12168>

    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.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12146>

    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.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12147>

    Another case where the result of xmpp_pubsub_iq_create() is not checked for NULL.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12148>

    Another missing NULL return check.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12175>

    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.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12171>

    6 is an odd thing to have here. Use a constant.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12172>

    The unlock here looks wrong. Where was client->buddies locked previously?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12170>

    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().

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12169>

    The comment says "semicolon" but the code is checking for a colon. Which is it supposed to be?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12166>

    Would ast_skip_blanks() work here?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12167>

    Odd spacing here.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12164>

    Why is this a warning?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12160>

    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.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12161>

    Is a NULL return here worthy of abandoning configuration of this client?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12162>

    Is a NULL return here worthy of abandoning configuration for this client?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12154>

    These error messages should either both mention both configuration files or else both only mention xmpp.conf

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12156>

    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.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12158>

    Since client flags depend on flags from the general section, you should change this to process the general section first and then each client.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12152>

    The "&& message" part of this if seems unnecessary since you've already bailed out earlier in the function if message is NULL.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12153>

    Shouldn't you check the return value of ast_xmpp_client_send_message() to determine if you send a Success or Error response?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12149>

    Another missing NULL return check.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12150>

    xmpp_pubsub_build_node_request() can return NULL. Is it safe to continue without checking for a NULL return?

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12151>

    Check for NULL return of xmpp_publish_build_node_request().

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12144>

    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.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12142>

    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.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12137>

    This should be changed to "Google Talk capable"

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12132>

    Nitpick: Use AST_MODULE_LOAD_SUCCESS here.

/trunk/res/res_xmpp.c
<https://reviewboard.asterisk.org/r/1983/#comment12133>

    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?

- Mark

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/20120611/23c67b5f/attachment-0001.htm>