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

[Srikanth Raju]

Hi,

I noticed that CommitTransaction needs a return code as commits can fail
and so does RollbackTransaction.
I've attached the patch for vlc_sql.h

On Wed, Sep 23, 2009 at 11:54 PM, Laurent Aimar <fenrir at via.ecp.fr> wrote:

>
> > +    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.
>
>
Not exactly sure how you want me to specify this. I have included that
the functions are threadsafe in the documentation in the first patch
(attached).


>
> > +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.
>
>
Correct. Makes no sense at all. Removed.


>
> 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.
>
>
Ok I've fixed this, I think.

I've locked the transaction queries using lock as well.
The trans_lock is to ensure that transactions are already not running from
the
given thread. This isn't actually "required", as the BeginTransaction will
fail
if a transaction is already running. But using mutexes for this makes the
task of running a transaction much easier, as the user will not have to
keep testing the return value of BeginTransaction until the transaction
starts.

lock was used for sqlite threadsafety, which should be there for all
queries,
including the transaction control queries.


>  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.
>
>
Not all sqlite are threadsafe. If the sqlite is compiled with the threadsafe
option,
then the functions are threadsafe. It is possible to compile sqlite
without thread safety. So, instead of testing for whether the given sqlite
is threadsafe
all the time and then locking using mutex conditionally, I have used locks
for all
queries. This shouldn't be too much of an overhead.


Regards,
>
> --
> fenrir
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>



-- 
Regards,
Srikanth Raju
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090924/626ce65d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Sql-pf_commit-and-pf_rollback-should-have-a-return.patch
Type: text/x-diff
Size: 4002 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090924/626ce65d/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Sqlite-Module.patch
Type: text/x-diff
Size: 16061 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090924/626ce65d/attachment-0001.patch>