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

[Tilghman Lesher]

> On 2010-08-10 14:08:01, Tilghman Lesher wrote:
> > /team/ivaxer/ast_storage/main/storage.c, lines 252-256
> > <https://reviewboard.asterisk.org/r/773/diff/6/?file=12144#file12144line252>
> >
> >     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.
> 
> ivaxer wrote:
>     Changed. Need I use locks here?

It's probably best to use locking, yes.


> On 2010-08-10 14:08:01, Tilghman Lesher wrote:
> > /team/ivaxer/ast_storage/res/res_storage_odbc.c, lines 597-599
> > <https://reviewboard.asterisk.org/r/773/diff/6/?file=12145#file12145line597>
> >
> >     You should be able to do all this initialization directly in the declaration.
> 
> ivaxer wrote:
>     Hm, why? BTW, GCC says: "ISO C90 forbids mixed declarations and code". I can fix this in some functions.. but why?

Correct, but what I was referring to was the ability to initialize directly in the declaration, as in:

struct get_query_data qdata = { .sql = sql, .objectname = objectname, .pathname = ost->pathname };


> On 2010-08-10 14:08:01, Tilghman Lesher wrote:
> > /team/ivaxer/ast_storage/res/res_storage_odbc.c, line 841
> > <https://reviewboard.asterisk.org/r/773/diff/6/?file=12145#file12145line841>
> >
> >     I think this needs to be fleshed out.
> 
> ivaxer wrote:
>     I have an idea. What if we expand functions, that take objectname as "const char*", to be able to specify the pass to object in storage medium?
>     
>     Code examples:
>     ast_storage_get(*st, "test_fileobject", NULL);
>     ast_storage_get(*st, "relative/path/test_fileobject", NULL);
>     ast_storage_get(*st, "/absolute/path/test_fileobject", NULL);
>     
>     P.S. In current implementation objectname must contains only logical name. The path stored in field ast_storage instance.

I think that's a good idea, as the general prefix is already stored within the storage object.  I don't know if a relative logical path would necessarily be used at all, but there are cases where we would want to be able to specify just the filename and have the prefix automatically prepended, and there are cases where we would want to be able to specify an absolute logical path.

The only places I would see as needing a relative path are those where we might want to add "../" to retract part of the default prefix, but that's a whole other problem to solve (including the possibility of security issues).


- Tilghman


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


On 2010-08-11 15:21:48, ivaxer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/773/
> -----------------------------------------------------------
> 
> (Updated 2010-08-11 15:21:48)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Generic file storage engine support
> 
> 
> Diffs
> -----
> 
>   /team/ivaxer/ast_storage/include/asterisk/_private.h 262099 
>   /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
> 
>