[vlc-devel] [PATCH] SQLite module

[Rémi Denis-Courmont]

+if test "${SYS}" != "darwin"; then
+  PKG_ENABLE_MODULES_VLC([SQLITE], [], [sqlite3], [sqlite3], [auto])
+else
+  AC_ARG_ENABLE(sqlite,
+    [  --enable-sqlite   SQLite (default disabled)])

AC_ARG_ENABLE is not supposed to be in a conditional.

+  if test "${enable_sqlite}" = "yes"

This check is wrong. It will fail with --enable-sqlite=foobar.

+  then
+    AC_ARG_WITH(sqlite,
+      [    --with-sqlite=PATH sqlite path linking])

Do we really need this? Also, same error as AC_ARG_ENABLE.


+struct sql_sys_t
+{
+    sqlite3 *db;              /**< Database connection. */
+    vlc_mutex_t lock;         /**< SQLite mutex. Threads are evil here. */
+    vlc_mutex_t trans_lock;   /**< Mutex for running transactions */
+};

Why do you need two locks?

+struct sql_stmt_t
+{
+    sqlite3_stmt* p_sqlitestmt;
+};

Using the SQLite pointer directly would be simpler.

+    /* Initialize sys_t */
+    p_sql->p_sys = calloc( 1, sizeof( *p_sql->p_sys ) );
+    if( !p_sql->p_sys )
+        return VLC_ENOMEM;
+
+    vlc_mutex_init( &p_sql->p_sys->lock );
+    vlc_mutex_init( &p_sql->p_sys->trans_lock );
+
+    /* Open Database */
+    if( OpenDatabase( p_sql ) == VLC_SUCCESS )
+        msg_Dbg( p_sql, "sqlite module loaded" );
+    else
+        return VLC_EGENERIC;

The error path is leaking.

+static int OpenDatabase( sql_t *p_sql )
+{
+    vlc_mutex_lock( &p_sql->p_sys->lock );

Why do you need the mutex here?

+    assert( !p_sql->p_sys->db );

For a pedant, this is wrong since you are zeroing bits instead of NULLifying 
the pointer.

+static int CloseDatabase( sql_t *p_sql )
+{
+    vlc_mutex_lock( &p_sql->p_sys->lock );

Why do you need the mutex here?

+    assert( p_sql->p_sys->db );
+
+    /* Close database */
+    int i_ret = sqlite3_close( p_sql->p_sys->db );
+    p_sql->p_sys->db = NULL;

What's the point? You're about to free p_sys anyway.

+    vlc_mutex_unlock( &p_sql->p_sys->lock );
+    return ( i_ret == SQLITE_OK ) ? VLC_SUCCESS : VLC_EGENERIC;

The API does not allow failing to close the database...

+static int Query( sql_t * p_sql,
+                  const char * query,
+                  char *** result,
+                  int * nrow,
+                  int * ncol )
+{
+    vlc_mutex_lock( &p_sql->p_sys->lock );
+
+    assert( p_sql->p_sys->db );
+
+#ifndef NDEBUG
+    msg_Dbg( p_sql, "Query: %s", query );
+#endif
+    sqlite3_get_table( p_sql->p_sys->db, query, result, nrow, ncol, NULL );
+    if( sqlite3_errcode( p_sql->p_sys->db ) != SQLITE_OK )
+    {
+        msg_Warn( p_sql, "sqlite3 error: %d: %s",
+                  sqlite3_errcode( p_sql->p_sys->db ),
+                  sqlite3_errmsg( p_sql->p_sys->db ) );
+        vlc_mutex_unlock( &p_sql->p_sys->lock );
+        return VLC_EGENERIC;
+    }
+
+    vlc_mutex_unlock( &p_sql->p_sys->lock );
+    return VLC_SUCCESS;
+}

You could really factor vlc_mutex_unlock() while adding a return value 
variable here and in latter functions.

+static int GetTables( sql_t * p_sql,
+                      char *** result )
+{
+    int nrow, i_num = false;

Assigning false to an integer is legal but weird.

+static char* VMSprintf( const char* psz_fmt, va_list args )
+{
+    char *psz = sqlite3_vmprintf( psz_fmt, args );
+    char *ret = strdup( psz );
+    sqlite3_free( psz );
+    return ret;
+}

Do we really need this double copy/allocation?

+static int CommitTransaction( sql_t* p_sql )
+{
+    assert( p_sql->p_sys->db );
+
+    vlc_mutex_lock( &p_sql->p_sys->lock );
+
+    /** This turns the auto commit on. */
+    sqlite3_exec( p_sql->p_sys->db, "COMMIT;", NULL, NULL, NULL );
+#ifndef NDEBUG
+    msg_Dbg( p_sql, "Transaction Query: COMMIT;" );
+#endif
+    if( sqlite3_errcode( p_sql->p_sys->db ) != SQLITE_OK )
+    {
+        msg_Warn( p_sql, "sqlite3 error: %d: %s",
+                  sqlite3_errcode( p_sql->p_sys->db ),
+                  sqlite3_errmsg( p_sql->p_sys->db ) );
+        vlc_mutex_unlock( &p_sql->p_sys->lock );

How does the caller recover from this? Is the transaction lock not stalled?

+static int RollbackTransaction( sql_t* p_sql )
+{
+    assert( p_sql->p_sys->db );
+
+    vlc_mutex_lock( &p_sql->p_sys->lock );
+
+    sqlite3_exec( p_sql->p_sys->db, "ROLLBACK;", NULL, NULL, NULL );
+#ifndef NDEBUG
+    msg_Dbg( p_sql, "Transaction Query: ROLLBACK;" );
+#endif
+    if( sqlite3_errcode( p_sql->p_sys->db ) != SQLITE_OK )
+    {
+        msg_Warn( p_sql, "sqlite3 error: %d: %s",
+                  sqlite3_errcode( p_sql->p_sys->db ),
+                  sqlite3_errmsg( p_sql->p_sys->db ) );
+        vlc_mutex_unlock( &p_sql->p_sys->lock );

Same stalling problem.

IMHO, RollbackTransaction should return void, because it does not make much 
sense to fail here.


+static sql_stmt_t* PrepareStatement( sql_t* p_sql, const char* psz_fmt, int 
i_length )
+{
+    assert( p_sql->p_sys->db );
+    vlc_mutex_lock( &p_sql->p_sys->lock );
+    sql_stmt_t* p_stmt;
+    p_stmt = calloc( 1, sizeof( *p_stmt ) );
+    if( p_stmt == NULL )
+    {
+        vlc_mutex_unlock( &p_sql->p_sys->lock );
+        return NULL;
+    }

Why do you need the lock to allocate memory?

-- 
Rémi Denis-Courmont
http://www.remlab.net/