Re: [PATCH 2/2] Added SQLite history plugin

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]

Hi Dario,

> +#define SQL_HISTORY_DB_PATH STORAGEDIR "/ofono_history.sqlite"
> +#define SQL_HISTORY_DB_SQL STORAGEDIR "/oFono_History_DB.sql"

So I have my concerns about storing this in /.  This should be in a proper 
directory, like /var/lib/ofono or something like that.

> +static int sqlite_history_probe(struct ofono_history_context *context)
> +{
> +	char *execerror;
> +
> +	ofono_debug("SQLite History Probe for modem: %p", context->modem);
> +
> +	if (sqlite3_open(SQL_HISTORY_DB_PATH, &db) != SQLITE_OK) {
> +		ofono_debug("Error opening DB: %s", sqlite3_errmsg(db));

Please use DBG macro instead of ofono_debug.

> +		sqlite3_close(db);
> +		return -1;
> +	}
> +
> +	if (sqlite3_exec(db, SELECT_CALLS, NULL, NULL, &execerror) != SQLITE_OK)
>  { +		char *sqlscript;

Should be

if (sqlite3_exec == SQLITE_OK)
	return 0;

Then the rest of the if statement follows unnested.
> +		GError *sqlerror = NULL;
> +
> +		ofono_debug("Creating DB");
> +
> +		g_file_get_contents(SQL_HISTORY_DB_SQL, &sqlscript, NULL, &sqlerror);
> +
> +		if (sqlerror != NULL) {
> +			ofono_debug("Error opening sql script: %s", sqlerror->message);
> +			g_error_free(sqlerror);
> +			return -1;
> +		}
> +
> +		if (sqlite3_exec(db, sqlscript, NULL, NULL, &execerror) != SQLITE_OK) {
> +			ofono_debug("Error executing sql script: %s", execerror);
> +			sqlite3_free(execerror);
> +			g_free(sqlscript);
> +			return -1;
> +		}
> +
> +		g_free(sqlscript);
> +	}
> +
> +	return 0;
> +}
> +
> +static void sqlite_history_remove(struct ofono_history_context *context)
> +{
> +	ofono_debug("SQLite History Remove for modem: %p", context->modem);
> +
> +	if (db != NULL)
> +		sqlite3_close(db);
> +}
> +

So remember that the history plugin is instantiated for each and every modem, 
so you will be sqlite_open/sqlite_closing the db in each instance.  Not really 
sure if this is what you want, or whether you want a reference-counted 
database connection here.

Another thing to consider is that you might want to store the IMSI of the 
modem along with the history information for every call / sms event.  That way 
when a SIM card is changed, the user can be shown a different set of call / sms  
history.

Finally, you might want to set limits on the number of entries in the 
database, and expire them as the limit is reached.   Otherwise you'd need to 
expire them periodically from e.g. cron, but would need to release control of 
the database during that time.

Regards,
-Denis