[asterisk-dev] [Code Review] Initial try at astobj2 of res_jabber

[Mark Michelson]

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



/trunk/channels/chan_gtalk.c
<http://reviewboard.digium.com/r/82/#comment360>

    It looks like you have a reference leak here. ao2_find (and ao2_t_find) increases the reference count of the object, and buddy's reference doesn't appear to be passed to any other object here.



/trunk/channels/chan_jingle.c
<http://reviewboard.digium.com/r/82/#comment361>

    This looks like the same problem I pointed out in gtalk_alloc



/trunk/include/asterisk/jabber.h
<http://reviewboard.digium.com/r/82/#comment362>

    This should be removed before merging, but you probably knew that already.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment364>

    You should use prime numbers for these values. It will allow for better distribution of hashed data. Also they should be const.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment363>

    What's the reason behind these changes?



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment365>

    For the comparison callbacks, it is generally considered good practice to return CMP_MATCH | CMP_STOP on a match.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment366>

    Since your comparison callbacks use case-insensitive comparisons, you should use a case-insensitive hash here. Use ast_str_case_hash.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment368>

    While I think this actually isn't harmful the way you have it here, it would be more clear if you used 'c' instead of 'obj' here.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment369>

    I believe it is possible for buddy to be NULL here. You should check this before unreffing it.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment371>

    You should be sure that buddy is non-NULL here.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment372>

    Be sure that buddy is not NULL here.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment373>

    Make sure that buddy is not NULL here.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment374>

    Again, be sure that buddy is not NULL. I'll stop saying that for the rest of this function, but there are two more places that need the check, still.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment375>

    You should move this lock up to its original place. There is a line above which references buddy->resources, and I assume that the lock should be held while that data is referenced.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment376>

    The cast isn't necessary here.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment377>

    This reference is not necessary since ao2_alloc already will set the reference count for the object to 1.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment378>

    This, however, is necessary since you really only want the container to contain a reference to the buddy.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment385>

    lol



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment393>

    Is this a separate enhancement/bug fix, or is this part of the conversion to ao2? I'm not seeing its relevance to this review request.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment389>

    Unref the local client pointer after linking it.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment390>

    You need to unref the buddy after linking it.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment391>

    It's good practice to unlock the buddy prior to unreffing it.



/trunk/res/res_jabber.c
<http://reviewboard.digium.com/r/82/#comment392>

    I believe you need to unref the clients container and the c->buddies container in here so that the containers are freed too.


Throughout the diff, I found several issues regarding whitespace. Look for the places where reviewboard puts a big red area and delete those from the diff. Also keep in mind that there should be a space after any if, while, for, do, or comma.

Thanks for all the hard work!

- Mark


On 2008-12-08 15:45:10, Brad Latus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/82/
> -----------------------------------------------------------
> 
> (Updated 2008-12-08 15:45:10)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is an attempt to convert jabber to ao2 objects.
> After this is successful i hope to merge another review for gtalk and jingle to ao2 objects as well.
> 
> Can also be found in branch /team/snuffy/ao2_jabber_take2
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_gtalk.c 161216 
>   /trunk/channels/chan_jingle.c 161216 
>   /trunk/include/asterisk/jabber.h 161216 
>   /trunk/res/res_jabber.c 161216 
> 
> Diff: http://reviewboard.digium.com/r/82/diff
> 
> 
> Testing
> -------
> 
> Very limited testing done. (connect to gtalk -> have msg sent to ast -> core stop gracefully)
> Ref counts do not match atm, off by 1..
> 
> 
> Thanks,
> 
> Brad
> 
>