[asterisk-dev] [Code Review] GSoC2010: ast_storage

[Tilghman Lesher]

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



/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5573>

    I would expect this to be at least the size of the pathname in the parent object, since this will be that length, plus the length of the extension.



/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5574>

    Add documentation stating what the return value will be.



/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5575>

    This prototype should probably be moved to _private.h, since it's only intended to be used by asterisk.c.



/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5576>

    Specify two \retval, one with NULL, one with non-NULL.



/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5577>

    Specify \retval here.



/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5578>

    Specify the exts parameter here, especially the format of the parameter.  Also, specify return values.



/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5579>

    Return value?



/team/ivaxer/ast_storage/main/file.c
<https://reviewboard.asterisk.org/r/773/#comment5580>

    Add braces



/team/ivaxer/ast_storage/main/file.c
<https://reviewboard.asterisk.org/r/773/#comment5581>

    Add braces



/team/ivaxer/ast_storage/main/storage.c
<https://reviewboard.asterisk.org/r/773/#comment5582>

    This should be you, not Mark, since you wrote this file.



/team/ivaxer/ast_storage/main/storage.c
<https://reviewboard.asterisk.org/r/773/#comment5583>

    Delete.  Mark didn't write this file; you did.



/team/ivaxer/ast_storage/main/storage.c
<https://reviewboard.asterisk.org/r/773/#comment5584>

    Delete.  It's already initialized, because you used calloc.



/team/ivaxer/ast_storage/main/storage.c
<https://reviewboard.asterisk.org/r/773/#comment5585>

    Change this from "already unregistered" to "not found in list (was it already unregistered?)".  If we try to unregister "bogus", then it should not say that it was unregistered, since it was never registered in the first place.



/team/ivaxer/ast_storage/main/storage.c
<https://reviewboard.asterisk.org/r/773/#comment5586>

    While you can use this construct, it's actually unnecessary.  You can instead just remove each item with AST_RWLIST_REMOVE_HEAD, which is equally safe.  The existing construct is for removing items out of the middle of a list safely.



/team/ivaxer/ast_storage/main/storage.c
<https://reviewboard.asterisk.org/r/773/#comment5587>

    Spacing.



/team/ivaxer/ast_storage/main/storage.c
<https://reviewboard.asterisk.org/r/773/#comment5588>

    Is this something that is going to be done this week?



/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5589>

    close local_fd here or we leak a file descriptor.



/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5590>

    You should be able to do all this initialization directly in the declaration.



/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5591>

    There is no need to cast to a void *.



/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5592>

    I think this needs to be fleshed out.


- Tilghman


On 2010-08-10 11:28:54, ivaxer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/773/
> -----------------------------------------------------------
> 
> (Updated 2010-08-10 11:28:54)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Generic file storage engine support
> 
> 
> Diffs
> -----
> 
>   /team/ivaxer/ast_storage/include/asterisk/file.h 262099 
>   /team/ivaxer/ast_storage/include/asterisk/storage.h PRE-CREATION 
>   /team/ivaxer/ast_storage/main/asterisk.c 262099 
>   /team/ivaxer/ast_storage/main/file.c 262099 
>   /team/ivaxer/ast_storage/main/storage.c PRE-CREATION 
>   /team/ivaxer/ast_storage/res/res_storage_odbc.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/773/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> ivaxer
> 
>