[Home]

Summary:ASTERISK-08550: [patch] SQLite3 resource
Reporter:Steven Sokol (ssokol)Labels:
Date Opened:2007-01-10 18:10:49.000-0600Date Closed:2011-06-07 14:03:15
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/Configuration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) func_sqlite.conf
( 1) res_sqlite3.c
( 2) res_sqlite3.h
Description:This adds SQLite3 as both a function (in exactly the same way func_odbc adds functions to the dialplan) and as a manager action (which returns data in exactly the same format the GetConfig command).

The included config file shows how to add SQL statements.  The docs for func_odbc tell it pretty much like it is.
Comments:By: Anthony LaMantia (alamantia) 2007-01-10 18:48:20.000-0600

ah, i will review these, one thing you should prob remove the references to func_odbc from the config file :)
By: Anthony LaMantia-2 (anthonyl) 2007-01-21 14:41:44.000-0600

how is this different from the res_sqlite in asterisk_addons?
By: Steven Sokol (ssokol) 2007-01-21 17:09:36.000-0600

1) It follows the same structure as func_odbc, which the version in addons does not do;

2) It includes manager support for access to data from SQLite which returns data in the same format as the config parser routines;

3) It was written from scratch and doesn't owe anything to the other res_sqlite, and I've signed a disclaimer so it can be included in the main Asterisk distribution rather than hanging out in addons;

4) It uses SQLite3 which is much better in terms of locking than SQLite2.  IIRC, the version in addons is SQLite2-based.
By: Serge Vecher (serge-v) 2007-01-29 08:58:47.000-0600

cool, it would be great to get some test results here
By: Russell Bryant (russell) 2007-03-13 15:49:16

This looks cool in general, but I am concerned about the amount of code duplication from func_odbc.  It would be nice if they could be combined in some way.
By: Russell Bryant (russell) 2007-03-13 15:49:47

Corydon, what do you think about this?
By: Tilghman Lesher (tilghman) 2007-03-14 13:31:32

Why not just use the SQLite ODBC driver?

http://www.ch-werner.de/sqliteodbc/
By: Russell Bryant (russell) 2007-04-03 18:54:13

Steve, if using the ODBC driver for SQLite won't suit your needs, let me know.  I'm closing this out for now.
By: Steven Sokol (ssokol) 2007-04-03 19:22:58

The great thing about SQLite is that it has no dependencies which makes it much easier to install this on embedded systems.  If your concern is the overlap between this and the ODBC code, what's the big deal?  Not like it takes up megabytes of storage.

Thanks, -S
By: Russell Bryant (russell) 2007-04-04 00:16:43

I'm not concerned with the number of lines of code we have in the tree.  The problem with duplicating code is that bugs fixed in one place often don't get fixed in the other.  The same goes when new features get added.  Even if these things do happen in both places, it takes twice as long to do so.
By: Russell Bryant (russell) 2007-04-23 16:36:15

Here are some code review comments:

1) If the point of this is to have this optimal for embedded systems, then it would probably be a good idea to redo the acf_sqlite_query struct to use stringfields instead of these large buffers.  As it stands, it takes up 4kB for each instance, plus whatever PATH_MAX is, which could be anything from 256 bytes to another 4kB.

2) I'm a little concerned with the implementation of sqlite_busy().  Do you really think sleeping 20 seconds on each try is necessary?  Tis means that a call to this dialplan function could block for up to 100 seconds.  Yikes.  I would think something like a one second wait each time would be more reasonable.

3) There are some pformatting issues that do not conform to coding guidelines.

static int func() {
... should be ...
static int func()
{

for (i=0; i<argc; i++){
... should be ...
for (i = 0; i < argc; i++) {

4) There are bunch of places where you used ast_strdup() which dynamically allocates memory, but then it is never free'd.  Most of these can probably just be ast_strdupa().

5) For the sake of performance, make it so the length of a string is only calculated once:

for (i = 0; i < strlen(coldata); i++) {
... changed to ...
unsigned int coldata_len = strlen(coldata);
for (i = 0; i < coldata_len; i ++) {

6) Do not declare variables in the middle of code.  In the following snippet:

pbx_builtin_setvar_helper(chan, "SQL_ERROR", NULL);

AST_DECLARE_APP_ARGS(values,
AST_APP_ARG(field)[100];
);
AST_DECLARE_APP_ARGS(args,
AST_APP_ARG(field)[100];
);

The call to pbx_builtin_setvar_helper() has to be after the args declarations.  This exists in at least one other place, too.

7) In the following section of code:

if (file[0] == '/') {
/* full path */
strncpy(dbpath, file, sizeof(file));
} else {
sprintf(dbpath, "/etc/asterisk/%s", file);
}

The 3rd argument to strncpy should have been "sizeof(file) - 1".  But, actually, it needs to be changed to ast_copy_string(dbpath, file, sizeof(file));

Also, that call to sprintf() needs to be snprintf().  There is no way to ensure that the file provided in the configuration is actually smaller than PATH_MAX.

This problem exists in more than one place.

8) t = value ? ast_strdupa(value) : "";

if (!s || !t) {

There is no reason to check the return value of ast_strdupa(), or any other alloca() based macro.  There is no way for you to detect failure here.

9) sfc = ast_malloc(sizeof(*sfc));
sfc->record_count = 0;
sfc->column_count = 0;

Every time you use a function that dynamically allocates memory, the code must handle it failing.

However, please check all of the places where these allocations are done.  I see some places where they can just be allocated on the stack instead.

10) Since a lot of this code came from Tilghman's func_odbc, please give him credit at the top of your file somewhere.

11) Unload module can be significantly simplified:

static int unload_module(void)
{
  struct acf_sqlite_query *query;

  AST_LIST_LOCK(&sqlite_queries);
  while ((query = AST_LIST_REMOVE_HEAD(&sqlite_queries, list))) {
     ast_custom_function_unregister(query->acf);
     free_acf_query(query);
  }
  AST_LIST_UNLOCK(&sqlite_queries);

  return 0;
}
By: Tilghman Lesher (tilghman) 2007-05-17 18:55:35

I would recommend staying away from the sleep() call completely.  I would suggest using usleep(1), instead, since it's obvious what you're trying to do is to yield the processor to a thread that is holding a lock and needs to release it before you can proceed.  The sleep() call creates issues in Asterisk, as it's generally implemented with SIGALRM and thus messes with the signal mask for the entire process (and therefore causes problems with concurrency).
By: Russell Bryant (russell) 2007-06-01 14:12:40

Issue suspended until code can be updated to address the various comments given in code review.