[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/#review931
-----------------------------------------------------------



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

    What's the benefit of having these structs separate?



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

    If we're not planning on fully implementing the security part of this right now, and the interfaces are versioned, is there a benefit of having it in the API now, or can we just remove it?



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

    The version doesn't need to be in the API call if you just register an ast_data_entry object that has the version, path, and handler info in it.
    
    Also, I still suggest passing an ast_module pointer with the registration.  That way you can adjust module reference counts when you use the callbacks.



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

    I mentioned in a previous review, "will order ever matter?" with respect to the use of astobj2.  Here is a situation to consider.
    
    In chan_sip (or chan_iax2), when representing data about a configured peer, a list of codecs is part of the data.  The order of these codecs is important, as it represents the order of preference between them.  The only want to do that with an unordered children container is to have codecs be a single element, as a string or something, and order them that way.
    
    This is something that needs to be carefully considered soon due to the potential impact it has on the implementation.



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

    use ast_strdupa(), don't check for failure



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

    Put the opening brace on its own line.



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

    no need to check for failure here


- Russell


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