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

[Rémi Denis-Courmont]

Le dimanche 23 août 2009 19:18:55 Srikanth Raju, vous avez écrit :
> Hello,
> I'm requesting a code review for the sqlite module.
>
> The Sqlite module requires you to have sqlite3 installed. Compile by
> configuring with --enable-sqlite.
>
> Currently tested for Linux and Mac OS(Thanks to Barry Wardell!)
>
> Most of the work was done in the vlc-ecp-project by Antoine Lejeune
> <phytos at videolan.org>, Jean-Philippe André <jpeg at videolan.org>, Rémi
> Duraffort <ivoire at videolan.org>, Adrien Maglo <magsoft at videolan.org>.
> Thanks!
>
> The media library module will utilise this module.

+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, [

Does SQLite not provide a .pc file, so that we could use pkg-config instead?
+/* SQL */
+typedef struct sql_t sql_t;
+typedef struct sql_sys_t sql_sys_t;
+

Please don't add typedefs to vlc_common.h. This is legacy.

+#ifndef _VLC_SQL_H
+# define _VLC_SQL_H

Please don't add identifiers with leading underscores. This is forbidden by 
the ISO C standard.

-BASE_SUBDIRS = dummy memcpy notify testsuite playlist stats osd xml probe
+BASE_SUBDIRS = dummy memcpy notify testsuite playlist stats osd xml probe sql

Do you _really_ need to add another Makefile in a new directory? This slows 
down the build process.

+    vlc_mutex_t trans_lock;   /**< Mutex to lock when transactions are */

I don't understand the comment.

+    p_sql->p_sys = malloc( sizeof( sql_sys_t ) );
+    if( !p_sql->p_sys )
+        return VLC_ENOMEM;
+    memset( p_sql->p_sys, 0, sizeof( sql_sys_t ) );

You can use calloc() instead of malloc()+memset().

+    vlc_object_set_destructor( p_sql, destructor );

A destructor function cannot be in a module.

+static void destructor( vlc_object_t* obj )
+{
+    sql_t *p_sql = (sql_t *)obj;
+    assert( p_sql );
+
+    module_unneed( p_sql, p_sql->p_module );
+}

A module cannot unload itself.

+static void unload( vlc_object_t *obj )
+{
+    sql_t *p_sql = (sql_t *)obj;
+
+    if( p_sql->p_sys )
+    {
+        CloseDatabase( p_sql );
+        vlc_mutex_destroy( &p_sql->p_sys->lock );
+        vlc_mutex_destroy( &p_sql->p_sys->trans_lock );
+        free( p_sql->p_sys->psz_host );
+        free( p_sql->p_sys );
+    }

The if statement is useless.

+    if( p_sql->p_sys->db )
+    {
+        msg_Warn( p_sql, "a database is already opened!" );
+        vlc_mutex_unlock( &p_sql->p_sys->lock );
+        return VLC_EGENERIC;
+    }

This should probably be asserted. I don't see a point in checking for broken 
callers at release run-time.

+    if( !p_sql->p_sys->psz_host || !*p_sql->p_sys->psz_host )
+    {
+        msg_Err( p_sql, "no database specified" );
+        vlc_mutex_unlock( &p_sql->p_sys->lock );
+        return VLC_EGENERIC;
+    }

Same here. And the SQL connection should probably be done during pf_open 
anyway.

+    if( !p_sql->p_sys->db )
+    {
+        vlc_mutex_unlock( &p_sql->p_sys->lock );
+        return VLC_SUCCESS; /* Success if nothing to do */
+    }

Closing the database from the pf_close callback would be much simpler.

+    vlc_mutex_lock( &p_sql->p_sys->lock );
+
+    if( !p_sql->p_sys->db )
+    {
+        vlc_mutex_unlock( &p_sql->p_sys->lock );
+        return VLC_EGENERIC;
+    }

Assert...

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