[vlc-devel] [PATCH] Sqlite Module for VLC, GSoC 09

[Laurent Aimar]

Hi,

On Wed, Sep 23, 2009, Srikanth Raju wrote:
> New patches attached.
> If there are no more problems with these patches, we should merge them
> with master. This will help the ML merge.

> From 97ca35323317a67e389a58a55a826b91481395fb Mon Sep 17 00:00:00 2001
> From: Srikanth Raju <srikiraju at gmail.com>
> Date: Wed, 16 Sep 2009 02:11:08 +0530
> Subject: [PATCH] Sql Core
> 
> This includes support for a SQL-based modules in Core VLC
 Merged by [e56fae2da2e4d19c318b449080cd474cadb1e0fa].
Thanks for your work.

For the second patch:
> +dnl
> +dnl SQLite
> +dnl
> +if test "${SYS}" != "darwin"; then
> +  PKG_ENABLE_MODULES_VLC([SQLITE], [], [sqlite3], [sqlite3], [auto])
> +else
> +  AC_ARG_ENABLE(sqlite,
> +    [  --enable-sqlite   SQLite (default disabled)])
> +  if test "${enable_sqlite}" = "yes"
> +  then
> +    AC_ARG_WITH(sqlite,
> +      [    --with-sqlite=PATH sqlite path linking])
> +    AC_CHECK_HEADERS(sqlite3.h, [
> +        VLC_ADD_PLUGIN([sqlite])
> +        if test "${with_sqlite}" != "no" -a -n "${with_sqlite}"; then
> +          AC_MSG_CHECKING(existence of sqlite directory in ${with_sqlite})
> +          real_sqlite="`cd ${with_sqlite} 2>/dev/null && pwd`"
> +          if test -z "${real_sqlite}"
> +          then
> +            dnl  The given directory can't be found
> +            AC_MSG_RESULT(no)
> +            AC_MSG_ERROR([cannot cd to ${with_sqlite}])
> +          fi
> +          VLC_ADD_CFLAGS([sqlite],[-I${with_sqlite}/include])
> +          VLC_ADD_LIBS([sqlite], [-L${with_sqlite}/lib -lsqlite3])
> +          AC_MSG_RESULT(yes)
> +        else
> +          VLC_ADD_LIBS([sqlite], [-lsqlite3])
> +        fi
> +        AC_DEFINE([SQLITE_MODULE], 1, [Define if you want to use SQLite module]) ],
> +        AC_MSG_ERROR([sqlite3 is required for sqlite module]) )
> +  fi
> +fi
> +AM_CONDITIONAL([HAVE_SQLITE], [test "${enable_sqlite}" = "yes"])
 Can someone confirm that this part is ok ?


> +#define SQLITE_DB_TEXT N_( "Filename of the SQLite database" )
> +#define SQLITE_DB_LONGTEXT N_( "Path to the file containing the SQLite database" )
 Unused.

> +/*****************************************************************************
> + * Private structures
> + *****************************************************************************/
> +struct sql_sys_t
> +{
> +    char *psz_host;           /**< Database host: SQLite DB filename */
 Unused in fact.

> +    sqlite3 *db;              /**< Database connection. */
> +    vlc_mutex_t lock;         /**< SQLite mutex. Threads are evil here. */
 I see it is used to protect some of the sql_t functions.
 If it is wanted to be able to call sql_t from multiple threads, I would like
first a patch to vlc_sql.h specifying those requirements.

> +/**
> + * @brief Load module
> + * @param obj Parent object
> + * @return VLC_SUCCESS or VLC_ENOMEM
> + */
> +static int load( vlc_object_t *p_this )
> +{
> +    sql_t *p_sql = (sql_t *) p_this;
> +
> +    p_sql->pf_query_callback = QueryCallback;
> +    p_sql->pf_get_tables = GetTables;
> +    p_sql->pf_query = Query;
> +    p_sql->pf_free = FreeResult;
> +    p_sql->pf_vmprintf = VMSprintf;
> +    p_sql->pf_begin = BeginTransaction;
> +    p_sql->pf_commit = CommitTransaction;
> +    p_sql->pf_rollback = RollbackTransaction;
 It would be better to initialize the functions after you are sure to
return VLC_SUCCESS.

> +    /* Initialize sys_t */
> +    p_sql->p_sys = ( sql_sys_t* ) calloc( 1, sizeof( sql_sys_t ) );
p_sql->p_sys = calloc( 1, sizeof(*p_sql->p_sys) ) is probably safer.

> +    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;
> +
> +    return VLC_SUCCESS;
> +}

> +static int OpenDatabase( sql_t *p_sql )
> +{
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
> +
> +    vlc_mutex_unlock( &p_sql->p_sys->lock );
> +    return VLC_SUCCESS;
> +}
> +
> +static int CloseDatabase( sql_t *p_sql )
> +{
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
> +
> +    vlc_mutex_unlock( &p_sql->p_sys->lock );
> +    return ( i_ret == SQLITE_OK ) ? VLC_SUCCESS : VLC_EGENERIC;
> +}
 The locking here is useless but can stay if you prefer.

> +static int Query( sql_t * p_sql,
> +                  const char * query,
> +                  char *** result,
> +                  int * nrow,
> +                  int * ncol )
> +{
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
> +
> +    if( !p_sql->p_sys->db )
 I think a assert() is better (and more consistant with Query()).

> +static int GetTables( sql_t * p_sql,
> +                      char *** result )
> +{
> +    int nrow, i_num = false;
> +    char ***pppsz_res;
> +
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
> +
> +    if( !p_sql->p_sys->db )
 Same.
> +
> +    if( result )
> +        pppsz_res = result;
> +    else
> +        pppsz_res = (char ***) malloc( sizeof( char ** ) );
 What is the purpose of pppsz_res ? Is there an interest to call
GetTables with result == NULL ? If so, documentation in vlc_sql.h is
welcome.

> +static char* VMSprintf( const char* psz_fmt, va_list args )
> +{
> +    char *psz = sqlite3_vmprintf( psz_fmt, args );
 You should probably test for NULL here.
> +    char *ret = strdup( psz );
> +    sqlite3_free( psz );
> +    return ret;
> +}

> +static int BeginTransaction( sql_t* p_sql )
> +{
> +    vlc_mutex_lock( &p_sql->p_sys->trans_lock );
> +    if( !p_sql->p_sys->db )
 assert ?
> +    {
> +        vlc_mutex_unlock( &p_sql->p_sys->trans_lock );
> +        return VLC_EGENERIC;
> +    }

I see that 2 locks are used:
 - lock for non transaction functions
 - trans_lock for transaction functions
I don't think that works as you want.

 Unless sqlite is already reentrant and no lock are needed, or it is not and
then non transaction and transaction functions probably need to be protected
by the same lock.

Regards,

-- 
fenrir