[asterisk-dev] [Code Review] data api gsoc2009

[Russell Bryant]

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



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/275/#comment2350>

    Please try your hardest to not make unnecessary changes.  I know it's tempting, but it makes patches harder to review.  :-)



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/275/#comment2351>

    I think the conversion of this __queues_show() function is interesting, and is another good example of how to use the API.  However, I think we should avoid making these changes in modules.  What I would rather focus on right now is the implementation of the callbacks.
    
    As for how CLI commands and manager actions would work with this new API, instead of modifying individual implementations like this, I think it would make more sense to try to come up with a generic CLI and manager handler that executes a query and formats the data in a way that is appropriate for that interface.  I think this "new" way would be in addition to the existing code.
    
    For the CLI commands, we could probably come up with some CLI command aliases that map "data get" CLI commands to the application CLI command.  For example, we could map "queues show" to "data get asterisk/application/app_queue/queues" or something.



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/275/#comment2352>

    I'm not sure that I have a specific suggestion at the moment, but I generally don't like really long #defines like this.
    
    One reason is that if you happen to get a compiler error, there is not an easy way to know which line contains the error.



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/275/#comment2353>

    There are some fields, such as this one, which are internal implementation details that really shouldn't be exported.  This one is not meaningful outside of channel.c



/trunk/main/data.c
<https://reviewboard.asterisk.org/r/275/#comment2355>

    Minor tweak ... we generally include system headers before asterisk headers, with the exception of asterisk.h.



/trunk/main/data.c
<https://reviewboard.asterisk.org/r/275/#comment2354>

    It is good practice to always use a prime number for the number of buckets in your hash table.  For some additional commentary on this subject, here is a link:
    
    http://www.concentric.net/~Ttwang/tech/primehash.htm



/trunk/main/data.c
<https://reviewboard.asterisk.org/r/275/#comment2357>

    Since these two items go together, I would put them in a struct:
    
    static struct {
        struct ao2_container *container;
        ast_rwlock_t lock;
    } root_data;



/trunk/main/data.c
<https://reviewboard.asterisk.org/r/275/#comment2359>

    The wording here may be confusion to people.  Instead of "the" module, perhaps saying "a" module, since we don't know which module is the problem at this point in the code.



/trunk/main/data.c
<https://reviewboard.asterisk.org/r/275/#comment2361>

    an atomic atomic?


- Russell


On 2009-07-20 16:12:43, Eliel Sardañons wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/275/
> -----------------------------------------------------------
> 
> (Updated 2009-07-20 16:12:43)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> This is the first review request for the Data API GSoC 2009 project.
> An architectural review is requested.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_queue.c 207420 
>   /trunk/include/asterisk/_private.h 207420 
>   /trunk/include/asterisk/channel.h 207420 
>   /trunk/include/asterisk/data.h PRE-CREATION 
>   /trunk/include/asterisk/xml.h 207420 
>   /trunk/main/asterisk.c 207420 
>   /trunk/main/channel.c 207420 
>   /trunk/main/data.c PRE-CREATION 
>   /trunk/main/xml.c 207420 
>   /trunk/tests/test_data.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/275/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Eliel
> 
>