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

[Laurent Aimar]

Hi,
On Tue, Sep 15, 2009, Srikanth Raju wrote:
> >  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.
> Removed the transaction keyword.
> >> +
> >> +    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.
> Done.

 Thanks,

 I think it would be better to split this patch in two:
 - one that add sql_t object/functions
 - one that add sqlite implementation of sql_t.
as the first one does not depend on the second one.
(I have not looked at the sqlite part)


> diff --git a/include/vlc_sql.h b/include/vlc_sql.h
> new file mode 100644
> index 0000000..50dc285
> --- /dev/null
> +++ b/include/vlc_sql.h
> @@ -0,0 +1,236 @@
> +
> +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;
> +
> +    /** Connection Data */
> +    char* psz_host;         /**< Location or host of the database */
> +    char* psz_user;         /**< Username used to connect to database */
> +    char* psz_pass;         /**< Password used to connect to database */
char *psz_ to be consistant.

> +    int i_port;             /**< Port on which database is running */
> +
> +    /** 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 * );
 Creating a typedef for the callback might be better as it will ease
its documentation.

> +/**
> + * @brief Create a new SQL object.
> + * @param p_this Parent object to attach the SQL object to.
> + * @param psz_host URL to the database
> + * @param i_port Port on which the database is running
> + * @param psz_user Username to access the database
> + * @param psz_pass Password for the database
> + * @return The VLC SQL object, type sql_t.
> + **/
> +#define sql_Create( a, b, c, d, e, f ) __sql_Create( VLC_OBJECT(a), b, c, d, e, f )
> +VLC_EXPORT( sql_t*, __sql_Create, ( vlc_object_t *p_this, char *psz_name,
> +            char* psz_host, int i_port, char* psz_user, char* psz_pass ) );
 Changing __sql_Create into sql_Create and moving the #define below would
avoid adding more functions starting with __. (A #undef in misc/sql.c will
also be needed).

> +++ b/src/misc/sql.c
> @@ -0,0 +1,92 @@
> +#if !defined( __LIBVLC__ )
> +  #error You are not libvlc or one of its plugins. You cannot include this file
> +#endif
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <vlc_common.h>
> +#include <vlc_sql.h>
> +#include <assert.h>
> +#include "libvlc.h"
> +
> +static void __sql_Destructor( vlc_object_t* obj );
> +
> +/*
> + * @brief Create an instance of an SQL connection
> + * @param p_this Parent object
> + * @param psz_name Name which is passed to module_need (not needed)
> + * @return p_sql created and attached, module loaded
> + */
> +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" );
 I dunno what is the difference with vlc_object_create but it is simpler.
The sql_t* cast is useless.

> +    if( !p_sql )
> +    {
> +        msg_Err( p_this, "unable to create sql object" );
> +        return NULL;
> +    }
> +    vlc_object_attach( p_sql, p_this );
> +
> +    p_sql->psz_host = strdup( psz_host );
> +    p_sql->psz_user = strdup( psz_user );
> +    p_sql->psz_pass = strdup( psz_pass );
> +    p_sql->i_port = i_port;
> +
> +    p_sql->p_module = module_need( p_sql, "sql", psz_name, false );
> +    if( !p_sql->p_module )
> +    {
 You loose the p_sql->psz_* memory.
> +        vlc_object_release( p_sql );
> +        msg_Err( p_this, "SQL provider not found" );
> +        return NULL;
> +    }
> +
> +    vlc_object_set_destructor( p_sql, __sql_Destructor );
 I am not sure that using vlc_object_set_destructor instead of a dedicated
function to destroy a sql_t object is good.
 At least with a dedicated function, it is easier to track when an object is
created and destroyed.
 Besides, a sql_t object will not be shared and so I don't think the refcounting
here is a good thing, it might give bad ideas to people.
 I would like to hear the opinions of other people on this one.

> +static void __sql_Destructor( vlc_object_t* obj )
 No need to have __.

Regards,

-- 
fenrir