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

[Rémi Duraffort]

Hello,
some small comments on the patch.


> diff --git a/modules/misc/sql/sqlite.c b/modules/misc/sql/sqlite.c

> +    /* Initialize sys_t */
> +    p_sql->p_sys = malloc( sizeof( sql_sys_t ) );
> +    if( !p_sql->p_sys )
> +        return VLC_ENOMEM;
> +    memset( p_sql->p_sys, 0, sizeof( sql_sys_t ) );
Using calloc instead of malloc+memset


> +/**
> + * @brief Unload module
> + * @param obj This sql_t object
> + * @return Nothing
> + */
> +static void unload( vlc_object_t *obj )
> +{
> +    sql_t *p_sql = (sql_t *)obj;
> +
> +    if( p_sql->p_sys )
> +    {
> +        CloseDatabase( p_sql );
> +        vlc_mutex_destroy( &p_sql->p_sys->lock );
> +        vlc_mutex_destroy( &p_sql->p_sys->trans_lock );
> +        free( p_sql->p_sys->psz_host );
> +        free( p_sql->p_sys );
> +    }
AFAIK p_sys can't be NULL in the unlod function as you already test for
it when loading the module.


> +/**
> + * @brief Open current database
> + * @param p_sql This sql_t object
> + * @return VLC_SUCCESS or VLC_EGENERIC
> + * @note You have to set current database first
> + */
> +static int OpenDatabase( sql_t *p_sql )
> +{
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
> +
> +    if( p_sql->p_sys->db )
> +    {
> +        msg_Warn( p_sql, "a database is already opened!" );
> +        vlc_mutex_unlock( &p_sql->p_sys->lock );
> +        return VLC_EGENERIC;
> +    }
> +
> +    if( !p_sql->p_sys->psz_host || !*p_sql->p_sys->psz_host )
> +    {
> +        msg_Err( p_sql, "no database specified" );
> +        vlc_mutex_unlock( &p_sql->p_sys->lock );
> +        return VLC_EGENERIC;
> +    }
> +
> +    if( sqlite3_open( p_sql->p_sys->psz_host, &p_sql->p_sys->db ) )
> +    {
> +        msg_Err( p_sql, "Can't open database : %s", p_sql->p_sys->psz_host );
> +        vlc_mutex_unlock( &p_sql->p_sys->lock );
> +        return VLC_EGENERIC;
> +    }
> +
> +    vlc_mutex_unlock( &p_sql->p_sys->lock );
> +    return VLC_SUCCESS;
> +}
You can factorize the code using a goto here. But that's not mandatory.


> +/**
> + * @brief Get tables in database
> + * @param p_sql This sql_t object
> + * @param result SQL query
> + * @return Number of elements
> + * You have to set and open current database first
> + */
> +static int GetTables( sql_t * p_sql,
> +                      char *** result )
> +{
> +    int nrow, i_num = false;
> +    char ***pppsz_res;
> +
> +    if( result )
> +        pppsz_res = result;
> +    else
> +        pppsz_res = (char ***) malloc( sizeof( char ** ) );
> +
> +    vlc_mutex_lock( &p_sql->p_sys->lock );
> +
> +    if( !p_sql->p_sys->db )
> +    {
> +        vlc_mutex_unlock( &p_sql->p_sys->lock );
> +        return VLC_EGENERIC;
> +    }
ppsz_res is leaked here no ?
I think that's better to do the test for p_sql->p_sys->db before
allocating ppsz_res.


Best regards.

-- 
Rémi Duraffort | ivoire