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

[Russell Bryant]

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



/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2173>

    I would have expected the put() callback to receive some kind of argument that specifies what field is being set.  Is the design of this part of it still in progress?



/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2174>

    I think both of these structures should have version fields to be able to detect ABI changes in the core.
    
    For an example of what I mean, see the security event code that I have up on reviewboard right now.  I use version fields for all of the structures defined as part of the API in security_event_defs.h.



/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2172>

    What is the value of a string registrar argument?  Is it for debugging purposes?
    
    I think it would be a good idea to have an optional ast_module argument, though.  That way, you can automatically handle updating the module reference count when you call a callback in a module.



/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2175>

    I think this function could use some additional documentation to describe when you would want to use this over the normal ast_data_get() API call.  Is it just for debugging?



/trunk/include/asterisk/xml.h
<http://reviewboard.digium.com/r/275/#comment2176>

    A picky comment, but can you make the formatting of each block consistent?
    
    I would prefer \brief on the 2nd line, and each item only 1 space from the '*'.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2177>

    Would it make sense to declare these as int32_t and uint32_t to be more explicit about the data type?



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2178>

    Will node order ever be important?  If so, the current astobj2 may not fit the bill, since you have no control over ordering amongst the items in the container.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2180>

    I would add a note that this gets initialized in ast_data_init(), or someone may come around and replace this with AST_RWLOCK_DEFINE_STATIC().
    
    I actually prefer not using constructor based initialization for globals, though, so I think this way is better.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2181>

    There is a small bug here.
    
    If you want to make the comparison not case sensitive, then you should use ast_str_case_hash() as the hash function.  Otherwise, you can run into problems.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2182>

    In general, inline functions are better for the sake of type safety.  However, by writing it this way, the lock debugging code will always report that the locking comes from these functions, instead of where it was _really_ being locked/unlocked.  So, I would just use #defines.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2183>

    use ast_strlen_zero() here instead.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2184>

    Use ast_debug()



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2185>

    Use ast_strdupa()



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2186>

    There is no benefit from checking the result of strdupa().  Just assume it was successful.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2187>

    There is a memory leak here.  You have to unref the container itself.
    
    Also, once you unref the container, the rest of this code is not necessary.  When the container gets destroyed, it will automatically unlink all elements that were still in the container.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2188>

    I wouldn't put a default case here.  It will force you to reconsider this block of code every time you add an entry to the enum if you don't, which is a good thing.



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2189>

    no need for this check



/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2191>

    I think it would be handy to make the test code a CLI command.  In fact, I think it would be a great idea to build up some test code in a test module for the tests directory, instead of here.


- Russell


On 2009-06-26 19:19:30, Eliel Sardañons wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/275/
> -----------------------------------------------------------
> 
> (Updated 2009-06-26 19:19:30)
> 
> 
> 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/include/asterisk/_private.h 203905 
>   /trunk/include/asterisk/data.h PRE-CREATION 
>   /trunk/include/asterisk/xml.h 203905 
>   /trunk/main/asterisk.c 203905 
>   /trunk/main/data.c PRE-CREATION 
>   /trunk/main/xml.c 203905 
> 
> Diff: http://reviewboard.digium.com/r/275/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Eliel
> 
>