Hi,<br><br><div class="gmail_quote">2009/11/11 Rémi Denis-Courmont <span dir="ltr"><<a href="mailto:remi@remlab.net">remi@remlab.net</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+if test "${SYS}" != "darwin"; then<br>
+ PKG_ENABLE_MODULES_VLC([SQLITE], [], [sqlite3], [sqlite3], [auto])<br>
+else<br>
+ AC_ARG_ENABLE(sqlite,<br>
+ [ --enable-sqlite SQLite (default disabled)])<br>
<br>
AC_ARG_ENABLE is not supposed to be in a conditional.<br>
<br>
+ if test "${enable_sqlite}" = "yes"<br>
<br>
This check is wrong. It will fail with --enable-sqlite=foobar.<br>
<br></blockquote><div>How do I fix this? This structure is used infinite other times.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+ then<br>
+ AC_ARG_WITH(sqlite,<br>
+ [ --with-sqlite=PATH sqlite path linking])<br>
<br>
Do we really need this? Also, same error as AC_ARG_ENABLE.<br></blockquote><div><br></div><div>Yes, because Darwin doesn't have the latest sqlite.</div><div>And with this patch, we once again need sqlite_next_stmt(), as we're using prepared statements</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
<br>
+struct sql_sys_t<br>
+{<br>
+ sqlite3 *db; /**< Database connection. */<br>
+ vlc_mutex_t lock; /**< SQLite mutex. Threads are evil here. */<br>
+ vlc_mutex_t trans_lock; /**< Mutex for running transactions */<br>
+};<br>
<br>
Why do you need two locks?<br>
<br></blockquote><div>One for transactions and one for threadsafe-ness.</div><div>Some sqlite builds can be non threadsafe. Better to be safe.</div><div><br></div><div>trans_lock ensures that no two threads can be running a transaction at one time. Again, same issue as above.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+struct sql_stmt_t<br>
+{<br>
+ sqlite3_stmt* p_sqlitestmt;<br>
+};<br>
<br>
Using the SQLite pointer directly would be simpler.<br></blockquote><div>Maybe. But sql_stmt_t is used in vlc_sql.h. It wouldn't go well with other sql modules </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
+ /* Initialize sys_t */<br>
+ p_sql->p_sys = calloc( 1, sizeof( *p_sql->p_sys ) );<br>
+ if( !p_sql->p_sys )<br>
+ return VLC_ENOMEM;<br>
+<br>
+ vlc_mutex_init( &p_sql->p_sys->lock );<br>
+ vlc_mutex_init( &p_sql->p_sys->trans_lock );<br>
+<br>
+ /* Open Database */<br>
+ if( OpenDatabase( p_sql ) == VLC_SUCCESS )<br>
+ msg_Dbg( p_sql, "sqlite module loaded" );<br>
+ else<br>
+ return VLC_EGENERIC;<br>
<br>
The error path is leaking.<br></blockquote><div><br></div><div>Yes. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
+static int OpenDatabase( sql_t *p_sql )<br>
+{<br>
+ vlc_mutex_lock( &p_sql->p_sys->lock );<br>
<br>
Why do you need the mutex here?</blockquote><div>Not required. Removed. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
+ assert( !p_sql->p_sys->db );<br>
<br>
For a pedant, this is wrong since you are zeroing bits instead of NULLifying<br>
the pointer.<br></blockquote><div>Hmm, although I don't understand how this matters, it still is the same...</div><div>changed it to p_sql->p_sys->db == NULL anyway</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
+static int CloseDatabase( sql_t *p_sql )<br>
+{<br>
+ vlc_mutex_lock( &p_sql->p_sys->lock );<br>
<br>
Why do you need the mutex here?<br>
<br></blockquote><div>Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+ assert( p_sql->p_sys->db );<br>
+<br>
+ /* Close database */<br>
+ int i_ret = sqlite3_close( p_sql->p_sys->db );<br>
+ p_sql->p_sys->db = NULL;<br>
<br>
What's the point? You're about to free p_sys anyway.<br>
<br>
+ vlc_mutex_unlock( &p_sql->p_sys->lock );<br>
+ return ( i_ret == SQLITE_OK ) ? VLC_SUCCESS : VLC_EGENERIC;<br>
<br>
The API does not allow failing to close the database...<br></blockquote><div>Hmm, yes.</div><div>Fixed it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
+static int Query( sql_t * p_sql,<br>
+ const char * query,<br>
+ char *** result,<br>
+ int * nrow,<br>
+ int * ncol )<br>
+{<br>
+ vlc_mutex_lock( &p_sql->p_sys->lock );<br>
+<br>
+ assert( p_sql->p_sys->db );<br>
+<br>
+#ifndef NDEBUG<br>
+ msg_Dbg( p_sql, "Query: %s", query );<br>
+#endif<br>
+ sqlite3_get_table( p_sql->p_sys->db, query, result, nrow, ncol, NULL );<br>
+ if( sqlite3_errcode( p_sql->p_sys->db ) != SQLITE_OK )<br>
+ {<br>
+ msg_Warn( p_sql, "sqlite3 error: %d: %s",<br>
+ sqlite3_errcode( p_sql->p_sys->db ),<br>
+ sqlite3_errmsg( p_sql->p_sys->db ) );<br>
+ vlc_mutex_unlock( &p_sql->p_sys->lock );<br>
+ return VLC_EGENERIC;<br>
+ }<br>
+<br>
+ vlc_mutex_unlock( &p_sql->p_sys->lock );<br>
+ return VLC_SUCCESS;<br>
+}<br>
<br>
You could really factor vlc_mutex_unlock() while adding a return value<br>
variable here and in latter functions.<br></blockquote><div><br></div><div>Yes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
+static int GetTables( sql_t * p_sql,<br>
+ char *** result )<br>
+{<br>
+ int nrow, i_num = false;<br>
<br>
Assigning false to an integer is legal but weird.<br>
<br></blockquote><div>Yup. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+static char* VMSprintf( const char* psz_fmt, va_list args )<br>
+{<br>
+ char *psz = sqlite3_vmprintf( psz_fmt, args );<br>
+ char *ret = strdup( psz );<br>
+ sqlite3_free( psz );<br>
+ return ret;<br>
+}<br>
<br>
Do we really need this double copy/allocation?<br></blockquote><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
+static int CommitTransaction( sql_t* p_sql )<br>
+{<br>
+ assert( p_sql->p_sys->db );<br>
+<br>
+ vlc_mutex_lock( &p_sql->p_sys->lock );<br>
+<br>
+ /** This turns the auto commit on. */<br>
+ sqlite3_exec( p_sql->p_sys->db, "COMMIT;", NULL, NULL, NULL );<br>
+#ifndef NDEBUG<br>
+ msg_Dbg( p_sql, "Transaction Query: COMMIT;" );<br>
+#endif<br>
+ if( sqlite3_errcode( p_sql->p_sys->db ) != SQLITE_OK )<br>
+ {<br>
+ msg_Warn( p_sql, "sqlite3 error: %d: %s",<br>
+ sqlite3_errcode( p_sql->p_sys->db ),<br>
+ sqlite3_errmsg( p_sql->p_sys->db ) );<br>
+ vlc_mutex_unlock( &p_sql->p_sys->lock );<br>
<br>
How does the caller recover from this? Is the transaction lock not stalled?<br></blockquote><div>The transaction lock(I assume you mean trans_lock) is not locked on failure</div><div><br></div><div>The caller needs to call this function again. COMMITs may fail when there are pending writes.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+static int RollbackTransaction( sql_t* p_sql )<br>
+{<br>
+ assert( p_sql->p_sys->db );<br>
+<br>
+ vlc_mutex_lock( &p_sql->p_sys->lock );<br>
+<br>
+ sqlite3_exec( p_sql->p_sys->db, "ROLLBACK;", NULL, NULL, NULL );<br>
+#ifndef NDEBUG<br>
+ msg_Dbg( p_sql, "Transaction Query: ROLLBACK;" );<br>
+#endif<br>
+ if( sqlite3_errcode( p_sql->p_sys->db ) != SQLITE_OK )<br>
+ {<br>
+ msg_Warn( p_sql, "sqlite3 error: %d: %s",<br>
+ sqlite3_errcode( p_sql->p_sys->db ),<br>
+ sqlite3_errmsg( p_sql->p_sys->db ) );<br>
+ vlc_mutex_unlock( &p_sql->p_sys->lock );<br>
<br>
Same stalling problem.<br>
<br>
IMHO, RollbackTransaction should return void, because it does not make much<br>
sense to fail here.<br></blockquote><div>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. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
<br>
+static sql_stmt_t* PrepareStatement( sql_t* p_sql, const char* psz_fmt, int<br>
i_length )<br>
+{<br>
+ assert( p_sql->p_sys->db );<br>
+ vlc_mutex_lock( &p_sql->p_sys->lock );<br>
+ sql_stmt_t* p_stmt;<br>
+ p_stmt = calloc( 1, sizeof( *p_stmt ) );<br>
+ if( p_stmt == NULL )<br>
+ {<br>
+ vlc_mutex_unlock( &p_sql->p_sys->lock );<br>
+ return NULL;<br>
+ }<br>
<br>
Why do you need the lock to allocate memory?<br>
<font color="#888888"><br></font></blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><font color="#888888">
--<br>
Rémi Denis-Courmont<br>
<a href="http://www.remlab.net/" target="_blank">http://www.remlab.net/</a><br>
</font></blockquote></div><br><br clear="all"><br>-- <br>Regards,<br>Srikanth Raju<br>