[vlc-devel] [PATCH] SQLite module

[Rémi Denis-Courmont]

	Hello,

Le mercredi 11 novembre 2009 20:46:46 Srikanth Raju, vous avez écrit :
> > How do I fix this? This structure is used infinite other times.

Read the fine (autoconf) manual.

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

What's wrong with contribs? Why do you need your own path?

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

If SQLite is not thread-safe, then your locking scheme won't work anyway. You 
would need to use a static lock and ensure nobody else is using SQLite in the 
process (best of luck with that).

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

I still don't see why do you need two *nested* locks...

The transaction lock is totally useless if you always acquire the other lock 
at the same time.

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

I fail to see a problem here. I never said you should expose SQLite typedefs 
outside.

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

That's not the point. calloc() does not warranty that db is NULL, it 
warranties that db is all zero bits. I would simply remove assertion.

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

That's not the point. Why do you need strdup and free?

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

I don't care what the SQLite people decided. There _must_ be a way to recover 
from the error. Keeping a stalled mutex is _not_ acceptable under any 
circumstances. In fact, it will cause an assertion failure when you close the 
DB.

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