[vlc-devel] [PATCH] SQLite module

[Srikanth Raju]

Hi,

2009/11/11 Rémi Denis-Courmont <remi at remlab.net>

> +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.
>
> How do I fix this? This structure is used infinite other times.


> +  then
> +    AC_ARG_WITH(sqlite,
> +      [    --with-sqlite=PATH sqlite path linking])
>
> Do we really need this? Also, same error as AC_ARG_ENABLE.
>

Yes, because Darwin doesn't have the latest sqlite.
And with this patch, we once again need sqlite_next_stmt(), as we're using
prepared statements


>
> +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?
>
> One for transactions and one for threadsafe-ness.
Some sqlite builds can be non threadsafe. Better to be safe.

trans_lock ensures that no two threads can be running a transaction at one
time. Again, same issue as above.

+struct sql_stmt_t
> +{
> +    sqlite3_stmt* p_sqlitestmt;
> +};
>
> Using the SQLite pointer directly would be simpler.
>
Maybe. But sql_stmt_t is used in vlc_sql.h. It wouldn't go well with other
sql modules

>
> +    /* 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.
>

Yes. Fixed.


>
> +static int OpenDatabase( sql_t *p_sql )
> +{
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
>
> Why do you need the mutex here?

Not required. Removed.


>
> +    assert( !p_sql->p_sys->db );
>
> For a pedant, this is wrong since you are zeroing bits instead of
> NULLifying
> the pointer.
>
Hmm, although I don't understand how this matters, it still is the same...
changed it to p_sql->p_sys->db == NULL anyway


>
> +static int CloseDatabase( sql_t *p_sql )
> +{
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
>
> Why do you need the mutex here?
>
> Removed.


> +    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...
>
Hmm, yes.
Fixed it.


>
> +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.
>

Yes.


>
> +static int GetTables( sql_t * p_sql,
> +                      char *** result )
> +{
> +    int nrow, i_num = false;
>
> Assigning false to an integer is legal but weird.
>
> Yup. Fixed.


> +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?
>
Yes. psz is allocated with sqlite3_malloc. Some "fastmem" builds can use
different allocation strategies, hence we cannot rely on free() to free psz.
Hence the copy.


>
> +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?
>
The transaction lock(I assume you mean trans_lock) is not locked on failure

The caller needs to call this function again. COMMITs may fail when there
are pending writes.



+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.
>
I agree that it should return void. However, the people who made sqlite
decided that a ROLLBACK can fail if there are pending reads/writes.


>
> +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?
>
> Fixed.

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



-- 
Regards,
Srikanth Raju
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20091112/2bc468a5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Sqlite-Module.patch
Type: application/octet-stream
Size: 26077 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20091112/2bc468a5/attachment.obj>