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

[Srikanth Raju]

Hello,

On Sat, Aug 29, 2009 at 1:03 AM, Laurent Aimar<fenrir at via.ecp.fr> wrote:
> Hi,
>
> On Sat, Aug 29, 2009, Srikanth Raju wrote:
>> +struct sql_t
>> +{
>> +    VLC_COMMON_MEMBERS
>> +
>> +    /** Module properties */
>> +    module_t  *p_module;
>> +
>> +    /** Internal data */
>> +    sql_sys_t *p_sys;
>> +
>> +    /** Set parameters for the database connection */
>> +    void (*pf_set_database) ( sql_t *, const char *,
>> +                              int , const char *, const char * );
>> +
>> +    /** Connect to the database previously configured */
>> +    int (*pf_open_database) ( sql_t * );
>> +
>> +    /** Close the connection */
>> +    int (*pf_close_database) ( sql_t * );
>
>  Is so many functions really needed ?
>
>  IMHO, merging pf_set_database and pf_open_database in the module Open
> and pf_close_database in the module Close function would simplify it
> without any loss. It would also prevent stupid usage mistakes.
>

Perhaps people don't want to create a connection until required?

>> +
>> +    /** Perform a query with a row-by-row callback function */
>> +    int (*pf_query_callback) ( sql_t *, const char *,
>> +            int (*callback) ( void*, int, char**, char** ), void * );
>> +
>> +    /** Perform a query and return result directly */
>> +    int (*pf_query) ( sql_t *, const char *, char ***, int *, int * );
>> +
>> +    /** Get database tables */
>> +    int (*pf_get_tables) ( sql_t *, char *** );
>> +
>
>> +    /** Free result of a call to sql_Query or sql_GetTables */
>> +    void (*pf_free) ( sql_t *, char ** );
>  I am not sure this one is really needed. Couldn't it be made independant
> of the module implementation ?
>

For the sqlite module, this uses a specific sqlite3_free_table.
I'm not sure if it's safe to free using C, neither does the documentation
explicitly allow it.

>> +
>> +    /** vmprintf replacement for SQL */
>> +    char* (*pf_vmprintf) ( const char*, va_list args );
>
>> +    /** Begin transaction */
>> +    int (*pf_begintransaction) ( sql_t* );
>> +
>> +    /** Commit transaction */
>> +    void (*pf_committransaction) ( sql_t* );
>> +
>> +    /** Rollback transaction */
>> +    void (*pf_rollbacktransaction) ( sql_t* );
>  Is the separation into 4 functions really needed ? And if so why ?
>

vmprintf is a printf like clone for sql that does quoting.

The other three are for transactions. We could have a single function
with a parameter for which transactional procedure to call. But is that really
easier? It's like having "vlc_mutex_proc(Lock) and vlc_mutex_proc(Unlock)"

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

Thanks!

-- 
Regards,
Srikanth Raju