-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/773/#review2467
-----------------------------------------------------------
/team/ivaxer/ast_storage/include/asterisk/storage.h
<https://reviewboard.asterisk.org/r/773/#comment5433>
Could you describe in a comment here, what the difference is between a fileinst and a fileobject?
If the difference is merely one so that you can have metadata, I'd propose that, instead, the metadata point to just another fileobject, instead.
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5434>
We generally do this, instead:
fo = ast_calloc(1, sizeof(*fo));
so that even if the declaration of fo changes, the right amount of space is still allocated and assigned.
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5435>
1) Use /tmp, not /var/tmp.
2) You should use a template that specifies something about what driver generated the temporary file, such as "/tmp/storage-odbc-XXXXXX". This is helpful later on, when you're trying to debug, so you can distinguish your temporary files from those left by another file.
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5436>
On most architectures, chunksize should not go below 4096, as this is the memory pagesize and the minimum amount for a mmap().
Pagesize will only be higher, not lower, on any system that will run Asterisk in the first place.
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5437>
Use ftruncate, when the file is still open.
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5438>
The last statement should also be within the conditional. If there are no extensions, then there are only two parameters to be bound.
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5439>
While this works for Voicemail, it will fail where metadata is NULL.
I understand the interest in making sure that both records are inserted in a single transaction, though, so perhaps the thing to do here is to prepare one of two different statements, the alternate of which is for the times when metadata is NULL.
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5440>
Add "const".
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5441>
"const"
/team/ivaxer/ast_storage/res/res_storage_odbc.c
<https://reviewboard.asterisk.org/r/773/#comment5442>
In this test, I'd like to see you create a metadata file and insert a standard file into the database. I'd additionally like you see you query the database and verify at the very least that the length of the metadata stored (and the binary sound, too) matches the length of the original file.
- Tilghman
On 2010-07-25 10:54:47, ivaxer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/773/
> -----------------------------------------------------------
>
> (Updated 2010-07-25 10:54:47)
>
>
> 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
>
>