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

[Laurent Aimar]

Hi,

On Tue, Sep 08, 2009, Srikanth Raju wrote:
> +/*****************************************************************************
> + * General structure: SQL object.
> + *****************************************************************************/
> +
> +typedef struct sql_t sql_t;
> +typedef struct sql_sys_t sql_sys_t;
> +
> +struct sql_t
> +{
> +    VLC_COMMON_MEMBERS
> +
> +    /** Module properties */
> +    module_t  *p_module;
> +
> +    /** Internal data */
> +    sql_sys_t *p_sys;
> +
> +    /** 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 ** );
> +
> +    /** 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* );
 I am not sure the 'transaction' part need to be in the function name. But if
you prefer to have it then I think it would be better to use _ to be consistent
with pf_query_callback and other vlc functions.

> +    /* Open Database */
> +    p_sql->p_sys->psz_host = var_CreateGetString( p_this, "database-path" );
 I think it is better to move the needed parameters to open the base into sql_t.
It will also allow to removes some unacceptable thinks later like:

> +sql_t *__sql_Create( vlc_object_t *p_this, char *psz_name, char* psz_host,
> +       int i_port, char* psz_user, char* psz_pass )
> +{
> +    sql_t *p_sql;
> +
> +    p_sql = ( sql_t * ) vlc_custom_create( p_this, sizeof( sql_t ),
> +                                           VLC_OBJECT_GENERIC, "sql" );
> +    if( !p_sql )
> +    {
> +        msg_Err( p_this, "unable to create sql object" );
> +        return NULL;
> +    }
> +    vlc_object_attach( p_sql, p_this );
> +
> +    config_PutPsz( p_sql, "database-path", psz_host );
> +    config_PutPsz( p_sql, "database-user", psz_user );
> +    config_PutPsz( p_sql, "database-pass", psz_pass );
> +    config_PutInt( p_sql, "database-port", i_port );
 Not acceptable, it need to be moved into sql_t.

Regards,

-- 
fenrir