Description of Voice call history plugin patch

Description of Voice call history plugin patch

[Rajyalakshmi Bommaraju]

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

							Call History Plugin 

Introduction:
Callhistory is a plugin loaded by ofono for persisting voice call history information in a disk file. Plugin exports dbus methods and signals as org.ofono.callhistory interface. Whenever there is a voice call in the system, ofono calls the plugin with the infomation (lineid,calltype,start time, end time) , plugin writes the information into the file , increments the 'unread' field in the header and sends out dbus signal with the number of unread records. Client (dialer) requests history records by calling method "GetVoiceHistory" exposed on the callhistory interface. After receiving information client calls the "SetVoiceHistoryRead" as an acknowledgement to indicate end of transaction. The pointers in the header are adjusted appropriately (explained clearly below).

Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.

Below is the file structure: (3916 bytes)

|Header|Data| 
0     16

Header Structure: (16 bytes)

|head|tail|unread|lastid|
0    4    8      12     16


head : Points to next slot for writing

tail : Points to next record to be read

unread : Number of Unread records so far

lastid : Number of the record that is last written 


Below is the structure of the history record:
History Record:(78 bytes)

|id|lineid|calltype|start time|end time|

id : Record id

lineid : phone Number

calltype : Outgoing/Incoming/Missed 

start time: start time in UTC

end time: end time in UTC

 
When the plugin is loaded by the ofono, it opens or creates a disk file of 3916 bytes size and maps it into fixed size memory space. Plugin reads the header of the file , if the 'unread' records is > 0 , a signal is sent out on the dbus with the information of number of unread records. 

Requesting data by client: 
When the client calls "GetVoiceHistory" method,plugin uses temp_tail and temp_unread variables to accomplish sending data to client. It copies tail pointer stored in the header into temporary tail pointer ,starts to read history records one at a time from the location pointed by the temp tail pointer , copies the into dbus structure, increments the temp tail pointer by size of the history record (78 bytes) and reads data from the next record location , this is repeated until it reaches head pointer. when all the unread records are read then the dbus structure is sent to the client. Once the data received, client needs to "SetVoiceHistoryRead" method to indicate that it received the data. Plugin will update the tail pointer with the temp tail pointer and unread records number is decremented by temp_unread. Using temp_tail and temp_unread variables protect from modifying the header if there is any loss in data delivery. If client doesnt call 'SetVoiceHistoryRead' then the actual header pointers wont be updated in the file. 

Writing record into the file:
When Ofono calls the plugin with call information (phone number, call type, call start time , call end time), plugin increaments the lastid to create record id for this record ,writes the record in the location pointed by head, and the head pointer is incremented by size of record (78 bytes), "unread" variable is incremented by 1. If the head reaches end of file (3916) then it is reset it back to begining of the data portion of the file (16 bytes). A dbus signal is sent out with number of current unread records. Plugin keeps writing the records in circular queue.

Locking: 
Accessing Memory mapped file pointer , all header fields (head,tail,unread,lastid), temp_unread,temp_tail are synchronized by a mutex. writing into memory file and reading from memory file happen asynchronously, so mutex is used to protect the memory mapped file pointer , header, temp_unread, temp_tail.

Limitations:
History is limited to storing 50 records. The above design assumes that there is only one client (either dialer or a history daemon) that reads data and sends acknowledgemnts. 

[PATCH] plugins: Adding implementation for persisting voice call history

[Rajyalakshmi Bommaraju]

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

---
 Makefile.am           |    3 +
 plugins/callhistory.c |  594 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 597 insertions(+), 0 deletions(-)
 create mode 100644 plugins/callhistory.c

diff --git a/Makefile.am b/Makefile.am
index aa66907..1b7f4d4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -261,6 +261,9 @@ builtin_sources += plugins/ste.c
 
 builtin_modules += caif
 builtin_sources += plugins/caif.c
+
+builtin_modules += callhistory
+builtin_sources += plugins/callhistory.c
 endif
 
 if MAINTAINER_MODE
diff --git a/plugins/callhistory.c b/plugins/callhistory.c
new file mode 100644
index 0000000..1579652
--- /dev/null
+++ b/plugins/callhistory.c
@@ -0,0 +1,594 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <string.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <glib.h>
+#include <time.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/dir.h>
+#include <sys/mman.h>
+#include <semaphore.h>
+#include <fcntl.h>
+#include <ofono/dbus.h>
+#include <dbus/dbus-glib.h>
+#include <ofono/types.h>
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/history.h>
+#include "gdbus.h"
+#include "common.h"
+
+#define HISTORY_FILE_PATH "/var/cache/callhistory/"
+#define HISTORY_FILE HISTORY_FILE_PATH"voicecallhistorydata"
+#define OFONO_MANAGER_PATH "/"
+#define PHONE_NUMBER_SIZE 64
+#define RECORD_SIZE 78
+#define HEADER_SIZE 16
+#define MAX_ITEMS 50
+
+#define TOTAL_SIZE (MAX_ITEMS * RECORD_SIZE + HEADER_SIZE)
+
+enum VOICE_CALL_TYPE {
+	OUTGOING = 0,
+	INCOMING,
+	MISSED
+};
+
+struct file_header {
+	int head;
+	int tail;
+	int unread;
+	unsigned int lastid;
+};
+
+struct voice_history {
+	int id;
+	char lineid[PHONE_NUMBER_SIZE];
+	short int calltype;
+	time_t starttime;
+	time_t endtime;
+};
+
+struct shared_info {
+	struct file_header header;
+	void *dataMap;
+	sem_t mutex;
+	int temp_unread;
+	int temp_tail;
+};
+
+static struct shared_info *shared_data;
+
+static int call_history_probe(struct ofono_history_context *context)
+{
+	DBG("History Probe for modem: %p", context->modem);
+	return 0;
+}
+
+static void call_history_remove(struct ofono_history_context *context)
+{
+	DBG("Example History Remove for modem: %p", context->modem);
+}
+
+static void clean_up()
+{
+	g_return_if_fail(shared_data != NULL);
+
+	sem_wait(&(shared_data->mutex));
+	/* unmap */
+	munmap(shared_data->dataMap, TOTAL_SIZE);
+	sem_post(&(shared_data->mutex));
+
+	/* remove semaphore */
+	if (sem_destroy(&(shared_data->mutex)) < 0)
+		perror("sem_destroy failed");
+
+	g_free(shared_data);
+}
+
+static int init_sem()
+{
+	unsigned int value = 1;
+	g_return_val_if_fail(shared_data != NULL, -1);
+
+	if (sem_init(&(shared_data->mutex), TRUE, value) < 0) {
+		perror("sem_init failed");
+		return -1;
+	}
+
+	return 0;
+}
+
+/* Start of DBus stuff */
+/* *************************************************************************
+ * Expose an interface, properties and signals for querying storage backend
+ * *************************************************************************/
+#define OFONO_CALL_HISTORY_INTERFACE OFONO_SERVICE".CallHistory"
+
+static void callhistory_emit_voice_history_changed(int userdata)
+{
+	int *valint = &userdata;
+	DBusConnection *conn = ofono_dbus_get_connection();
+	g_dbus_emit_signal(conn,
+		OFONO_MANAGER_PATH,
+		OFONO_CALL_HISTORY_INTERFACE,
+		"VoiceHistoryChanged",
+		DBUS_TYPE_UINT32,
+		valint,
+		DBUS_TYPE_INVALID);
+}
+
+static DBusMessage *call_history_set_voice_history_read(DBusConnection *conn,
+							DBusMessage *msg,
+							void *userdata)
+{
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	g_return_val_if_fail((shared_data != NULL), NULL);
+
+	ofono_debug("Read ack received");
+
+	reply = dbus_message_new_method_return(msg);
+
+	if (!reply)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+	sem_wait(&(shared_data->mutex));
+	shared_data->header.unread = (shared_data->header).unread -
+					shared_data->temp_unread;
+	shared_data->header.tail = shared_data->temp_tail;
+	/* write to file */
+	memcpy(shared_data->dataMap, &(shared_data->header), HEADER_SIZE);
+	sem_post(&(shared_data->mutex));
+	msync(shared_data->dataMap, TOTAL_SIZE, MS_ASYNC);
+	return reply;
+}
+
+static int init_header(void *dataPtr)
+{
+	int unread = 0;
+
+	g_return_val_if_fail((shared_data != NULL), -1);
+	g_return_val_if_fail((dataPtr != NULL), -1);
+	unread = (shared_data->header).unread;
+
+	sem_wait(&(shared_data->mutex));
+	memcpy(&(shared_data->header), (struct file_header *)(dataPtr),
+							HEADER_SIZE);
+
+	ofono_debug("Unread: %d, Header: %d, Tail: %d, serialno: %d\n",
+					unread,
+					(shared_data->header).head,
+					(shared_data->header).tail,
+					(shared_data->header).lastid);
+	sem_post(&(shared_data->mutex));
+	if (unread > 0)
+		callhistory_emit_voice_history_changed(unread);
+
+	return 0;
+}
+
+static void sync_mem_file(struct voice_history *vcall)
+{
+	void *writeMap = NULL;
+	int unread = 0;
+
+	g_return_if_fail(vcall != NULL);
+	g_return_if_fail(shared_data != NULL);
+	g_return_if_fail(shared_data->dataMap != NULL);
+
+	sem_wait(&(shared_data->mutex));
+
+	writeMap = shared_data->dataMap;
+	writeMap = writeMap + (shared_data->header).head;
+	vcall->id = (shared_data->header).lastid;
+	(shared_data->header).lastid++;
+	memcpy(writeMap, vcall, RECORD_SIZE);
+
+	/* update header */
+	((shared_data->header).unread)++;
+	(shared_data->header).head += RECORD_SIZE;
+
+	if ((shared_data->header).lastid >= UINT_MAX)
+		(shared_data->header).lastid = 0;
+
+	unread = (shared_data->header).unread;
+
+	/* header reaches end of file */
+	if ((shared_data->header).head >= TOTAL_SIZE) {
+		/* reset head back to front */
+		(shared_data->header).head = HEADER_SIZE;
+
+		ofono_debug("Header = %d, Tail = %d",
+				(shared_data->header).head,
+				(shared_data->header).tail);
+
+		if (unread >= MAX_ITEMS)
+			ofono_error("No one reading history data");
+
+	}
+
+	memcpy(shared_data->dataMap, &(shared_data->header), HEADER_SIZE);
+	msync(shared_data->dataMap, TOTAL_SIZE, MS_ASYNC);
+
+	sem_post(&(shared_data->mutex));
+
+	callhistory_emit_voice_history_changed(unread);
+}
+
+/**
+ * Creates file  "callhistory"
+ */
+static gboolean init_file()
+{
+	int historyFile;
+	struct stat statbuf;
+	DIR *dirptr;
+	char *fill = NULL;
+
+	g_return_val_if_fail((shared_data != NULL), FALSE);
+
+	dirptr = opendir(HISTORY_FILE_PATH);
+
+	if (!dirptr) {
+		ofono_debug("%s: doesnt exist", HISTORY_FILE_PATH);
+
+		if (mkdir(HISTORY_FILE_PATH,
+			S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH) < 0) {
+			perror("Error creating callhistory dir");
+			return FALSE;
+		}
+
+	}
+
+	historyFile = open(HISTORY_FILE,
+				O_RDWR|O_CREAT|O_APPEND,
+				S_IRWXU);
+	if (historyFile < 0) {
+		perror("Open file failed");
+		return FALSE;
+	}
+
+	if (stat(HISTORY_FILE, &statbuf) < -1) {
+		perror("stat failed");
+		return FALSE;
+	}
+
+	ofono_debug("stat file: %d", (int) (statbuf.st_size));
+
+	if (statbuf.st_size == 0) {
+		int byteswritten = 0;
+		/* write header to the file */
+		(shared_data->header).head = HEADER_SIZE;
+		(shared_data->header).tail = (shared_data->header).head;
+		(shared_data->header).unread = 0;
+		ofono_debug("setting head: %d, tail: %d, unread: %d",
+						(shared_data->header).head,
+						(shared_data->header).tail,
+						(shared_data->header).unread);
+
+		/*  fill the file with zeros */
+		ofono_debug("Trying to allocate %d size", TOTAL_SIZE);
+		fill = (char *) g_try_malloc0(TOTAL_SIZE);
+
+		if (!fill) {
+			ofono_debug("Error allocating init memory");
+			return FALSE;
+		}
+
+		/* Predefine file size and fill with some data */
+		bzero(fill, TOTAL_SIZE);
+		memcpy(fill, &(shared_data->header), HEADER_SIZE);
+
+		byteswritten = write(historyFile, fill, TOTAL_SIZE);
+
+		if (byteswritten < 0) {
+			ofono_debug("Error writing to file");
+			return FALSE;
+		}
+
+		g_free(fill);
+
+		statbuf.st_size = TOTAL_SIZE;
+		ofono_debug("History file %d size created\n", byteswritten);
+	}
+
+	/* create semaphor */
+	if (init_sem() < 0)
+		return FALSE;
+
+	sem_wait(&(shared_data->mutex));
+	shared_data->dataMap = mmap((caddr_t)NULL,
+				statbuf.st_size,
+				PROT_READ|PROT_WRITE|PROT_EXEC,
+				MAP_SHARED,
+				historyFile,
+				0);
+	sem_post(&(shared_data->mutex));
+
+	if (shared_data->dataMap == (void *)(-1)) {
+		perror("mmap falied");
+		return FALSE;
+	}
+
+	close(historyFile);
+
+	init_header(shared_data->dataMap);
+
+	return TRUE;
+}
+
+/**
+ * call_history_call_ended:
+ * ofono calls this method with the call information
+ */
+static void call_history_call_ended(struct ofono_history_context *context,
+					const struct ofono_call *call,
+					time_t start,
+					time_t end)
+{
+	const char *from = "Unknown";
+	struct tm tmstart, tmend;
+	char sttime[128], endtime[128];
+	int fromlen = 0;
+	struct voice_history vcall;
+
+	ofono_debug("Call Ended on modem: %p", context->modem);
+
+	if (call->type != 0)
+		return;
+
+	ofono_debug("Voice Call, %s",
+		call->direction ? "Incoming" : "Outgoing");
+
+	if (call->direction)
+		vcall.calltype = INCOMING;
+	else
+		vcall.calltype = OUTGOING;
+
+	if (call->clip_validity == 0)
+		from = phone_number_to_string(&call->phone_number);
+
+	fromlen = strlen(from);
+	strcpy(vcall.lineid, from);
+
+	vcall.lineid[fromlen] = '\0';
+
+	vcall.starttime = start;
+	gmtime_r(&start, &tmstart);
+	strftime(sttime, 127, "%a, %d %b %Y %H:%M:%S %z", &tmstart);
+	sttime[127] = '\0';
+
+	ofono_debug("StartTime: %s", sttime);
+
+	vcall.endtime = end;
+	gmtime_r(&end, &tmend);
+	strftime(endtime, 127, "%a, %d %b %Y %H:%M:%S %z", &tmend);
+	endtime[127] = '\0';
+	ofono_debug("EndTime: %s", endtime);
+
+	sync_mem_file(&vcall);
+}
+/**
+ * call_history_call_missed:
+ * ofono calls this method with the call information
+ */
+static void call_history_call_missed(struct ofono_history_context *context,
+					const struct ofono_call *call,
+					time_t when)
+{
+	const char *from = "Unknown";
+	struct tm mtime;
+	char sttime[128];
+	struct voice_history vcall;
+	int fromlen = 0;
+
+	ofono_debug("Call Missed on modem: %p", context->modem);
+
+	if (call->type != 0)
+		return;
+
+	ofono_debug("Voice Call, Missed");
+	ofono_debug("Voice Call, %s",
+		call->direction ? "Incoming" : "Outgoing");
+
+	vcall.calltype = MISSED;
+
+	if (call->clip_validity == 0)
+		from = phone_number_to_string(&call->phone_number);
+
+	fromlen = strlen(from);
+	strncpy(vcall.lineid, from, fromlen);
+	vcall.lineid[fromlen] = '\0';
+
+	vcall.starttime = when;
+
+	gmtime_r(&when, &mtime);
+	ofono_debug("From: %s", vcall.lineid);
+	strftime(sttime, 127, "%a, %d %b %Y %H:%M:%S %z", &mtime);
+	sttime[127] = '\0';
+	ofono_debug("Missed Time: %s", sttime);
+
+	vcall.endtime = when;
+
+	sync_mem_file(&vcall);
+}
+
+static void read_data(struct voice_history *data, int temp)
+{
+	void *readMap = NULL;
+	g_return_if_fail(shared_data != NULL);
+	g_return_if_fail(shared_data->dataMap != NULL);
+
+	sem_wait(&(shared_data->mutex));
+	readMap = shared_data->dataMap+temp;
+
+	/* read a chunk into *data */
+	memcpy(data, readMap, RECORD_SIZE);
+	shared_data->temp_unread++;
+	sem_post(&(shared_data->mutex));
+}
+
+static DBusMessage *call_history_get_voice_history(DBusConnection *conn,
+						DBusMessage *msg,
+						void *userdata)
+{
+	DBusMessage     *reply;
+	DBusMessageIter  iter;
+	DBusMessageIter  array, struct_s;
+	int i;
+	char *lineid;
+	struct voice_history data;
+	int unread = 0;
+
+	reply = dbus_message_new_method_return(msg);
+
+	if (!reply)
+		return NULL;
+
+	g_return_val_if_fail(shared_data != NULL, NULL);
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					DBUS_STRUCT_BEGIN_CHAR_AS_STRING
+					DBUS_TYPE_UINT32_AS_STRING
+					DBUS_TYPE_STRING_AS_STRING
+					DBUS_TYPE_UINT16_AS_STRING
+					DBUS_TYPE_INT32_AS_STRING
+					DBUS_TYPE_INT32_AS_STRING
+					DBUS_STRUCT_END_CHAR_AS_STRING
+					, &array);
+
+	sem_wait(&(shared_data->mutex));
+	shared_data->temp_unread = 0;
+
+	shared_data->temp_tail = (shared_data->header).tail;
+	unread = (shared_data->header).unread;
+	sem_post(&(shared_data->mutex));
+
+	for (i = 0; i < unread; i++) {
+		read_data(&data, shared_data->temp_tail);
+		lineid = data.lineid;
+		dbus_message_iter_open_container(&array,
+						DBUS_TYPE_STRUCT,
+						NULL,
+						&struct_s);
+		dbus_message_iter_append_basic(&struct_s,
+						DBUS_TYPE_UINT32,
+						&(data.id));
+		dbus_message_iter_append_basic(&struct_s,
+						DBUS_TYPE_STRING,
+						&(lineid));
+		dbus_message_iter_append_basic(&struct_s,
+						DBUS_TYPE_UINT16,
+						&(data.calltype));
+		dbus_message_iter_append_basic(&struct_s,
+						DBUS_TYPE_INT32,
+						&(data.starttime));
+		dbus_message_iter_append_basic(&struct_s,
+						DBUS_TYPE_INT32,
+						&(data.endtime));
+		dbus_message_iter_close_container(&array, &struct_s);
+
+		shared_data->temp_tail = shared_data->temp_tail + RECORD_SIZE;
+
+		if (shared_data->temp_tail == TOTAL_SIZE) {
+			ofono_debug("End of Queue: %d",
+					shared_data->temp_tail);
+			/* reset back to front */
+			shared_data->temp_tail = HEADER_SIZE;
+		}
+
+	}
+	dbus_message_iter_close_container(&iter, &array);
+
+	return reply;
+}
+
+static GDBusMethodTable call_history_methods[] = {
+	{ "GetVoiceHistory", "", "a(usqii)", call_history_get_voice_history },
+	{ "SetVoiceHistoryRead", "", "", call_history_set_voice_history_read },
+	{ }
+};
+
+static GDBusSignalTable call_history_signals[] = {
+	{ "VoiceHistoryChanged", "u" },
+	{}
+};
+
+/* End of DBus stuff */
+static struct ofono_history_driver call_history_driver = {
+	.name = "callhistory",
+	.probe = call_history_probe,
+	.remove = call_history_remove,
+	.call_ended = call_history_call_ended,
+	.call_missed = call_history_call_missed,
+};
+
+static int call_history_init(void)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+
+	if (!shared_data) {
+		shared_data = g_try_new0(struct shared_info, 1);
+
+		if (!shared_data)
+			return -ENOMEM;
+	}
+
+	if (!g_dbus_register_interface(conn,
+					OFONO_MANAGER_PATH,
+					OFONO_CALL_HISTORY_INTERFACE,
+					call_history_methods,
+					call_history_signals,
+					NULL,    /* Properties */
+					shared_data,    /* Userdata   */
+					NULL))    /* Destroy func */
+		return -EIO;
+
+	if (!init_file())
+		return -ENOENT;
+
+	return ofono_history_driver_register(&call_history_driver);
+}
+
+static void call_history_exit(void)
+{
+	clean_up();
+	ofono_history_driver_unregister(&call_history_driver);
+}
+
+OFONO_PLUGIN_DEFINE(callhistory, "Call History Plugin",
+		OFONO_VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+		call_history_init, call_history_exit)
-- 
1.6.3.3

Re: [PATCH] plugins: Adding implementation for persisting voice call history

[Marcel Holtmann]

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

Hi Rajyalakshmi,

>  Makefile.am           |    3 +
>  plugins/callhistory.c |  594 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 597 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/callhistory.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index aa66907..1b7f4d4 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -261,6 +261,9 @@ builtin_sources += plugins/ste.c
>  
>  builtin_modules += caif
>  builtin_sources += plugins/caif.c
> +
> +builtin_modules += callhistory
> +builtin_sources += plugins/callhistory.c
>  endif
>  
>  if MAINTAINER_MODE
> diff --git a/plugins/callhistory.c b/plugins/callhistory.c
> new file mode 100644
> index 0000000..1579652
> --- /dev/null
> +++ b/plugins/callhistory.c
> @@ -0,0 +1,594 @@
> +/*
> + *
> + * oFono - Open Source Telephony
> + *
> + * Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */

the indentation is broken here. Please fix that. The copyright notice
needs to be consistent throughout the whole source code.

> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <glib.h>
> +#include <time.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/dir.h>
> +#include <sys/mman.h>
> +#include <semaphore.h>
> +#include <fcntl.h>
> +#include <ofono/dbus.h>
> +#include <dbus/dbus-glib.h>
> +#include <ofono/types.h>
> +#include <ofono/plugin.h>
> +#include <ofono/log.h>
> +#include <ofono/history.h>
> +#include "gdbus.h"
> +#include "common.h"

Yikes, please sort the includes properly like all other code pieces in
oFono do.

First the system includes, the OFONO_API_SUBJECT_TO_CHANGE and the
required oFono includes, then GLib and/or GDBus, then private ones.

Also please only include the ones that are actually needed.

> +#define HISTORY_FILE_PATH "/var/cache/callhistory/"
> +#define HISTORY_FILE HISTORY_FILE_PATH"voicecallhistorydata"
> +#define OFONO_MANAGER_PATH "/"

What is this for?

> +#define PHONE_NUMBER_SIZE 64
> +#define RECORD_SIZE 78
> +#define HEADER_SIZE 16
> +#define MAX_ITEMS 50
> +
> +#define TOTAL_SIZE (MAX_ITEMS * RECORD_SIZE + HEADER_SIZE)
> +
> +enum VOICE_CALL_TYPE {
> +	OUTGOING = 0,
> +	INCOMING,
> +	MISSED
> +};
> +
> +struct file_header {
> +	int head;
> +	int tail;
> +	int unread;
> +	unsigned int lastid;
> +};

No. If these are files that are written to disk or memory or wherever
please use explicit sized variables like uint16 or similar.

You code is currently not compatible with 32-bit / 64-bit differences
and that is a bad idea.

> +struct voice_history {
> +	int id;
> +	char lineid[PHONE_NUMBER_SIZE];
> +	short int calltype;
> +	time_t starttime;
> +	time_t endtime;
> +};
> +
> +struct shared_info {
> +	struct file_header header;
> +	void *dataMap;
> +	sem_t mutex;
> +	int temp_unread;
> +	int temp_tail;
> +};

Are any of these stored on disk or in memory? If so they need to use
proper agnostic types.

> +static struct shared_info *shared_data;
> +
> +static int call_history_probe(struct ofono_history_context *context)
> +{
> +	DBG("History Probe for modem: %p", context->modem);
> +	return 0;
> +}
> +
> +static void call_history_remove(struct ofono_history_context *context)
> +{
> +	DBG("Example History Remove for modem: %p", context->modem);
> +}
> +
> +static void clean_up()
> +{
> +	g_return_if_fail(shared_data != NULL);
> +
> +	sem_wait(&(shared_data->mutex));
> +	/* unmap */
> +	munmap(shared_data->dataMap, TOTAL_SIZE);
> +	sem_post(&(shared_data->mutex));
> +
> +	/* remove semaphore */
> +	if (sem_destroy(&(shared_data->mutex)) < 0)
> +		perror("sem_destroy failed");
> +
> +	g_free(shared_data);
> +}
> +
> +static int init_sem()
> +{
> +	unsigned int value = 1;
> +	g_return_val_if_fail(shared_data != NULL, -1);
> +
> +	if (sem_init(&(shared_data->mutex), TRUE, value) < 0) {
> +		perror("sem_init failed");
> +		return -1;
> +	}
> +
> +	return 0;
> +}

Why is this here and not next to the _init and _exit functions where it
is actually used.

Also no perror stuff. We have connman_error. In addition the val_if_fail
checks are rather pointless since the plugin _init and _exit is
protected and guaranteed to be only called once.

> +
> +/* Start of DBus stuff */
> +/* *************************************************************************
> + * Expose an interface, properties and signals for querying storage backend
> + * *************************************************************************/
> +#define OFONO_CALL_HISTORY_INTERFACE OFONO_SERVICE".CallHistory"
> +
> +static void callhistory_emit_voice_history_changed(int userdata)
> +{
> +	int *valint = &userdata;
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	g_dbus_emit_signal(conn,
> +		OFONO_MANAGER_PATH,
> +		OFONO_CALL_HISTORY_INTERFACE,
> +		"VoiceHistoryChanged",
> +		DBUS_TYPE_UINT32,
> +		valint,
> +		DBUS_TYPE_INVALID);
> +}

What is this &valint hack here. That doesn't even look like it would
work properly. And even if it does, it is totally unneeded. Please use
the proper dbus_uint32_t types and just assign it. Later on you can just
use the &val value.

> +static DBusMessage *call_history_set_voice_history_read(DBusConnection *conn,
> +							DBusMessage *msg,
> +							void *userdata)
> +{
> +	DBusMessage *reply;
> +	DBusMessageIter iter;
> +	g_return_val_if_fail((shared_data != NULL), NULL);
> +
> +	ofono_debug("Read ack received");

No direct usage of ofono_debug. Use DBG() instead.

> +
> +	reply = dbus_message_new_method_return(msg);
> +
> +	if (!reply)
> +		return NULL;

Remove the empty line between the new_method and its error check.

> +
> +	dbus_message_iter_init_append(reply, &iter);
> +	sem_wait(&(shared_data->mutex));
> +	shared_data->header.unread = (shared_data->header).unread -
> +					shared_data->temp_unread;
> +	shared_data->header.tail = shared_data->temp_tail;
> +	/* write to file */
> +	memcpy(shared_data->dataMap, &(shared_data->header), HEADER_SIZE);

The extra () around shared_data->header are confusing. Create a temp
variable and use the header directly actually. And in case of &
operation you don't even need them.

> +	sem_post(&(shared_data->mutex));
> +	msync(shared_data->dataMap, TOTAL_SIZE, MS_ASYNC);
> +	return reply;
> +}
> +
> +static int init_header(void *dataPtr)
> +{
> +	int unread = 0;

Don't initialize variables until really needed. In this case it is
clearly not needed.

> +	g_return_val_if_fail((shared_data != NULL), -1);
> +	g_return_val_if_fail((dataPtr != NULL), -1);

What are these for. How can shared_data be NULL. How can dataPtr be
NULL.

And while at it dataPtr is not a valid variable name according to our
coding style. Please fix that.

> +	unread = (shared_data->header).unread;

A temporary variable for the header would make this a bit better.

> +
> +	sem_wait(&(shared_data->mutex));
> +	memcpy(&(shared_data->header), (struct file_header *)(dataPtr),
> +							HEADER_SIZE);

No need for () around dataPtr. This is (struct file_header *) dataPtr.

> +
> +	ofono_debug("Unread: %d, Header: %d, Tail: %d, serialno: %d\n",
> +					unread,
> +					(shared_data->header).head,
> +					(shared_data->header).tail,
> +					(shared_data->header).lastid);
> +	sem_post(&(shared_data->mutex));
> +	if (unread > 0)
> +		callhistory_emit_voice_history_changed(unread);
> +
> +	return 0;
> +}
> +
> +static void sync_mem_file(struct voice_history *vcall)
> +{
> +	void *writeMap = NULL;
> +	int unread = 0;
> +
> +	g_return_if_fail(vcall != NULL);
> +	g_return_if_fail(shared_data != NULL);
> +	g_return_if_fail(shared_data->dataMap != NULL);
> +
> +	sem_wait(&(shared_data->mutex));
> +
> +	writeMap = shared_data->dataMap;
> +	writeMap = writeMap + (shared_data->header).head;
> +	vcall->id = (shared_data->header).lastid;
> +	(shared_data->header).lastid++;
> +	memcpy(writeMap, vcall, RECORD_SIZE);
> +
> +	/* update header */
> +	((shared_data->header).unread)++;
> +	(shared_data->header).head += RECORD_SIZE;
> +
> +	if ((shared_data->header).lastid >= UINT_MAX)
> +		(shared_data->header).lastid = 0;
> +
> +	unread = (shared_data->header).unread;
> +
> +	/* header reaches end of file */
> +	if ((shared_data->header).head >= TOTAL_SIZE) {
> +		/* reset head back to front */
> +		(shared_data->header).head = HEADER_SIZE;
> +
> +		ofono_debug("Header = %d, Tail = %d",
> +				(shared_data->header).head,
> +				(shared_data->header).tail);
> +
> +		if (unread >= MAX_ITEMS)
> +			ofono_error("No one reading history data");
> +
> +	}
> +
> +	memcpy(shared_data->dataMap, &(shared_data->header), HEADER_SIZE);
> +	msync(shared_data->dataMap, TOTAL_SIZE, MS_ASYNC);
> +
> +	sem_post(&(shared_data->mutex));
> +
> +	callhistory_emit_voice_history_changed(unread);
> +}
> +
> +/**
> + * Creates file  "callhistory"
> + */
> +static gboolean init_file()
> +{
> +	int historyFile;
> +	struct stat statbuf;
> +	DIR *dirptr;
> +	char *fill = NULL;
> +
> +	g_return_val_if_fail((shared_data != NULL), FALSE);
> +
> +	dirptr = opendir(HISTORY_FILE_PATH);
> +
> +	if (!dirptr) {
> +		ofono_debug("%s: doesnt exist", HISTORY_FILE_PATH);
> +
> +		if (mkdir(HISTORY_FILE_PATH,
> +			S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH) < 0) {
> +			perror("Error creating callhistory dir");
> +			return FALSE;
> +		}
> +
> +	}

See all my comments above for this one. This is rather too complex in
what it tries to achieve. And it uses wrong perror, ofono_debug etc.

> +
> +	historyFile = open(HISTORY_FILE,
> +				O_RDWR|O_CREAT|O_APPEND,
> +				S_IRWXU);
> +	if (historyFile < 0) {
> +		perror("Open file failed");
> +		return FALSE;
> +	}
> +
> +	if (stat(HISTORY_FILE, &statbuf) < -1) {
> +		perror("stat failed");
> +		return FALSE;
> +	}
> +
> +	ofono_debug("stat file: %d", (int) (statbuf.st_size));
> +
> +	if (statbuf.st_size == 0) {
> +		int byteswritten = 0;
> +		/* write header to the file */
> +		(shared_data->header).head = HEADER_SIZE;
> +		(shared_data->header).tail = (shared_data->header).head;
> +		(shared_data->header).unread = 0;
> +		ofono_debug("setting head: %d, tail: %d, unread: %d",
> +						(shared_data->header).head,
> +						(shared_data->header).tail,
> +						(shared_data->header).unread);
> +
> +		/*  fill the file with zeros */
> +		ofono_debug("Trying to allocate %d size", TOTAL_SIZE);
> +		fill = (char *) g_try_malloc0(TOTAL_SIZE);
> +
> +		if (!fill) {
> +			ofono_debug("Error allocating init memory");
> +			return FALSE;
> +		}
> +
> +		/* Predefine file size and fill with some data */
> +		bzero(fill, TOTAL_SIZE);
> +		memcpy(fill, &(shared_data->header), HEADER_SIZE);
> +
> +		byteswritten = write(historyFile, fill, TOTAL_SIZE);
> +
> +		if (byteswritten < 0) {
> +			ofono_debug("Error writing to file");
> +			return FALSE;
> +		}
> +
> +		g_free(fill);
> +
> +		statbuf.st_size = TOTAL_SIZE;
> +		ofono_debug("History file %d size created\n", byteswritten);
> +	}
> +
> +	/* create semaphor */
> +	if (init_sem() < 0)
> +		return FALSE;
> +
> +	sem_wait(&(shared_data->mutex));
> +	shared_data->dataMap = mmap((caddr_t)NULL,
> +				statbuf.st_size,
> +				PROT_READ|PROT_WRITE|PROT_EXEC,
> +				MAP_SHARED,
> +				historyFile,
> +				0);
> +	sem_post(&(shared_data->mutex));
> +
> +	if (shared_data->dataMap == (void *)(-1)) {
> +		perror("mmap falied");
> +		return FALSE;
> +	}
> +
> +	close(historyFile);
> +
> +	init_header(shared_data->dataMap);
> +
> +	return TRUE;
> +}

This at least contains a file descriptor leak.

> +/**
> + * call_history_call_ended:
> + * ofono calls this method with the call information
> + */
> +static void call_history_call_ended(struct ofono_history_context *context,
> +					const struct ofono_call *call,
> +					time_t start,
> +					time_t end)
> +{
> +	const char *from = "Unknown";
> +	struct tm tmstart, tmend;
> +	char sttime[128], endtime[128];
> +	int fromlen = 0;
> +	struct voice_history vcall;
> +
> +	ofono_debug("Call Ended on modem: %p", context->modem);
> +
> +	if (call->type != 0)
> +		return;
> +
> +	ofono_debug("Voice Call, %s",
> +		call->direction ? "Incoming" : "Outgoing");
> +
> +	if (call->direction)
> +		vcall.calltype = INCOMING;
> +	else
> +		vcall.calltype = OUTGOING;
> +
> +	if (call->clip_validity == 0)
> +		from = phone_number_to_string(&call->phone_number);
> +
> +	fromlen = strlen(from);
> +	strcpy(vcall.lineid, from);
> +
> +	vcall.lineid[fromlen] = '\0';
> +
> +	vcall.starttime = start;
> +	gmtime_r(&start, &tmstart);
> +	strftime(sttime, 127, "%a, %d %b %Y %H:%M:%S %z", &tmstart);
> +	sttime[127] = '\0';
> +
> +	ofono_debug("StartTime: %s", sttime);
> +
> +	vcall.endtime = end;
> +	gmtime_r(&end, &tmend);
> +	strftime(endtime, 127, "%a, %d %b %Y %H:%M:%S %z", &tmend);
> +	endtime[127] = '\0';
> +	ofono_debug("EndTime: %s", endtime);
> +
> +	sync_mem_file(&vcall);
> +}
> +/**
> + * call_history_call_missed:
> + * ofono calls this method with the call information
> + */
> +static void call_history_call_missed(struct ofono_history_context *context,
> +					const struct ofono_call *call,
> +					time_t when)
> +{
> +	const char *from = "Unknown";
> +	struct tm mtime;
> +	char sttime[128];
> +	struct voice_history vcall;
> +	int fromlen = 0;
> +
> +	ofono_debug("Call Missed on modem: %p", context->modem);
> +
> +	if (call->type != 0)
> +		return;
> +
> +	ofono_debug("Voice Call, Missed");
> +	ofono_debug("Voice Call, %s",
> +		call->direction ? "Incoming" : "Outgoing");
> +
> +	vcall.calltype = MISSED;
> +
> +	if (call->clip_validity == 0)
> +		from = phone_number_to_string(&call->phone_number);
> +
> +	fromlen = strlen(from);
> +	strncpy(vcall.lineid, from, fromlen);
> +	vcall.lineid[fromlen] = '\0';
> +
> +	vcall.starttime = when;
> +
> +	gmtime_r(&when, &mtime);
> +	ofono_debug("From: %s", vcall.lineid);
> +	strftime(sttime, 127, "%a, %d %b %Y %H:%M:%S %z", &mtime);
> +	sttime[127] = '\0';
> +	ofono_debug("Missed Time: %s", sttime);
> +
> +	vcall.endtime = when;
> +
> +	sync_mem_file(&vcall);
> +}
> +
> +static void read_data(struct voice_history *data, int temp)
> +{
> +	void *readMap = NULL;
> +	g_return_if_fail(shared_data != NULL);
> +	g_return_if_fail(shared_data->dataMap != NULL);
> +
> +	sem_wait(&(shared_data->mutex));
> +	readMap = shared_data->dataMap+temp;
> +
> +	/* read a chunk into *data */
> +	memcpy(data, readMap, RECORD_SIZE);
> +	shared_data->temp_unread++;
> +	sem_post(&(shared_data->mutex));
> +}
> +
> +static DBusMessage *call_history_get_voice_history(DBusConnection *conn,
> +						DBusMessage *msg,
> +						void *userdata)
> +{
> +	DBusMessage     *reply;
> +	DBusMessageIter  iter;
> +	DBusMessageIter  array, struct_s;
> +	int i;
> +	char *lineid;
> +	struct voice_history data;
> +	int unread = 0;
> +
> +	reply = dbus_message_new_method_return(msg);
> +
> +	if (!reply)
> +		return NULL;
> +
> +	g_return_val_if_fail(shared_data != NULL, NULL);
> +
> +	dbus_message_iter_init_append(reply, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					DBUS_STRUCT_BEGIN_CHAR_AS_STRING
> +					DBUS_TYPE_UINT32_AS_STRING
> +					DBUS_TYPE_STRING_AS_STRING
> +					DBUS_TYPE_UINT16_AS_STRING
> +					DBUS_TYPE_INT32_AS_STRING
> +					DBUS_TYPE_INT32_AS_STRING
> +					DBUS_STRUCT_END_CHAR_AS_STRING
> +					, &array);
> +
> +	sem_wait(&(shared_data->mutex));
> +	shared_data->temp_unread = 0;
> +
> +	shared_data->temp_tail = (shared_data->header).tail;
> +	unread = (shared_data->header).unread;
> +	sem_post(&(shared_data->mutex));
> +
> +	for (i = 0; i < unread; i++) {
> +		read_data(&data, shared_data->temp_tail);
> +		lineid = data.lineid;
> +		dbus_message_iter_open_container(&array,
> +						DBUS_TYPE_STRUCT,
> +						NULL,
> +						&struct_s);
> +		dbus_message_iter_append_basic(&struct_s,
> +						DBUS_TYPE_UINT32,
> +						&(data.id));
> +		dbus_message_iter_append_basic(&struct_s,
> +						DBUS_TYPE_STRING,
> +						&(lineid));
> +		dbus_message_iter_append_basic(&struct_s,
> +						DBUS_TYPE_UINT16,
> +						&(data.calltype));
> +		dbus_message_iter_append_basic(&struct_s,
> +						DBUS_TYPE_INT32,
> +						&(data.starttime));
> +		dbus_message_iter_append_basic(&struct_s,
> +						DBUS_TYPE_INT32,
> +						&(data.endtime));
> +		dbus_message_iter_close_container(&array, &struct_s);
> +
> +		shared_data->temp_tail = shared_data->temp_tail + RECORD_SIZE;
> +
> +		if (shared_data->temp_tail == TOTAL_SIZE) {
> +			ofono_debug("End of Queue: %d",
> +					shared_data->temp_tail);
> +			/* reset back to front */
> +			shared_data->temp_tail = HEADER_SIZE;
> +		}
> +
> +	}
> +	dbus_message_iter_close_container(&iter, &array);
> +
> +	return reply;
> +}
> +
> +static GDBusMethodTable call_history_methods[] = {
> +	{ "GetVoiceHistory", "", "a(usqii)", call_history_get_voice_history },
> +	{ "SetVoiceHistoryRead", "", "", call_history_set_voice_history_read },
> +	{ }
> +};
> +
> +static GDBusSignalTable call_history_signals[] = {
> +	{ "VoiceHistoryChanged", "u" },
> +	{}
> +};
> +
> +/* End of DBus stuff */
> +static struct ofono_history_driver call_history_driver = {
> +	.name = "callhistory",
> +	.probe = call_history_probe,
> +	.remove = call_history_remove,
> +	.call_ended = call_history_call_ended,
> +	.call_missed = call_history_call_missed,
> +};
> +
> +static int call_history_init(void)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +
> +	if (!shared_data) {
> +		shared_data = g_try_new0(struct shared_info, 1);
> +
> +		if (!shared_data)
> +			return -ENOMEM;
> +	}

Double init of plugin is not possible. Also you are leaking conn here in
case of errors.

> +	if (!g_dbus_register_interface(conn,
> +					OFONO_MANAGER_PATH,
> +					OFONO_CALL_HISTORY_INTERFACE,
> +					call_history_methods,
> +					call_history_signals,
> +					NULL,    /* Properties */
> +					shared_data,    /* Userdata   */
> +					NULL))    /* Destroy func */
> +		return -EIO;
> +
> +	if (!init_file())
> +		return -ENOENT;
> +
> +	return ofono_history_driver_register(&call_history_driver);

Interface registration should be done in the probe callback of the
history driver and not here. You are trying to work around race
conditions that you otherwise wouldn't have to begin with.

> +}
> +
> +static void call_history_exit(void)
> +{
> +	clean_up();
> +	ofono_history_driver_unregister(&call_history_driver);
> +}

Where do you unregister the D-Bus interface?

Regards

Marcel

Re: Description of Voice call history plugin patch

[Denis Kenzior]

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

Hi Raji,

> Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.

Just a couple quick points.  Memory mapped files do not really give any
benefit over read/write/lseek for small files (e.g. less than a few
pages) with small record sizes (e.g. less than a 4K page.)  In fact
they're probably slower in these cases.

> Locking: 
> Accessing Memory mapped file pointer , all header fields (head,tail,unread,lastid), temp_unread,temp_tail are synchronized by a mutex. writing into memory file and reading from memory file happen asynchronously, so mutex is used to protect the memory mapped file pointer , header, temp_unread, temp_tail.

I'm totally confused by this one; you use no threads in your plugin (and
neither does oFono) and you have no external clients accessing the same
shared memory region.  It seems to me that locking is completely
unnecessary.

Am I missing something here?

Regards,
-Denis

RE: Description of Voice call history plugin patch

[Bommaraju, Rajyalakshmi]

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

Denis,

Thanks for the feedback, I have given explanation for locking below.

Thanks,
Raji
-----Original Message-----
From: Denis Kenzior [mailto:denkenz(a)gmail.com] 
Sent: Thursday, September 16, 2010 8:17 PM
To: ofono(a)ofono.org
Cc: Bommaraju, Rajyalakshmi
Subject: Re: Description of Voice call history plugin patch

Hi Raji,

> Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.

Just a couple quick points.  Memory mapped files do not really give any
benefit over read/write/lseek for small files (e.g. less than a few
pages) with small record sizes (e.g. less than a 4K page.)  In fact
they're probably slower in these cases.

> Locking: 
> Accessing Memory mapped file pointer , all header fields (head,tail,unread,lastid), temp_unread,temp_tail are synchronized by a mutex. writing into memory file and reading from memory file happen asynchronously, so mutex is used to protect the memory mapped file pointer , header, temp_unread, temp_tail.

I'm totally confused by this one; you use no threads in your plugin (and
neither does oFono) and you have no external clients accessing the same
shared memory region.  It seems to me that locking is completely
unnecessary.

Raji > History reads are client driven, can happen during the write operation. Same with writing, if there is any read operation in progress, write operation needs to wait. So the mutex will lock the memory information until read/write operation is done. 

RE: Description of Voice call history plugin patch

[Marcel Holtmann]

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

Hi Raji,

> > Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.
> 
> Just a couple quick points.  Memory mapped files do not really give any
> benefit over read/write/lseek for small files (e.g. less than a few
> pages) with small record sizes (e.g. less than a 4K page.)  In fact
> they're probably slower in these cases.
> 
> > Locking: 
> > Accessing Memory mapped file pointer , all header fields (head,tail,unread,lastid), temp_unread,temp_tail are synchronized by a mutex. writing into memory file and reading from memory file happen asynchronously, so mutex is used to protect the memory mapped file pointer , header, temp_unread, temp_tail.
> 
> I'm totally confused by this one; you use no threads in your plugin (and
> neither does oFono) and you have no external clients accessing the same
> shared memory region.  It seems to me that locking is completely
> unnecessary.
> 
> Raji > History reads are client driven, can happen during the write operation. Same with writing, if there is any read operation in progress, write operation needs to wait. So the mutex will lock the memory information until read/write operation is done. 

who reads from this file. The mutex is local to ofonod. If any other
program reads from that memory you need a proper memory lock. The local
mutex is not helping since ofonod is (on purpose) single threaded and
thus no locking is actually needed since read and write operations from
ofonod can't happen at the same time.

Regards

Marcel

Re: Description of Voice call history plugin patch

[Marcel Holtmann]

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

Hi Raji,

> Callhistory is a plugin loaded by ofono for persisting voice call history information in a disk file. Plugin exports dbus methods and signals as org.ofono.callhistory interface. Whenever there is a voice call in the system, ofono calls the plugin with the infomation (lineid,calltype,start time, end time) , plugin writes the information into the file , increments the 'unread' field in the header and sends out dbus signal with the number of unread records. Client (dialer) requests history records by calling method "GetVoiceHistory" exposed on the callhistory interface. After receiving information client calls the "SetVoiceHistoryRead" as an acknowledgement to indicate end of transaction. The pointers in the header are adjusted appropriately (explained clearly below).
> 
> Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.

I am not buying into this performance argument at all. How many calls to
you expect per second.
 
> When the plugin is loaded by the ofono, it opens or creates a disk file of 3916 bytes size and maps it into fixed size memory space. Plugin reads the header of the file , if the 'unread' records is > 0 , a signal is sent out on the dbus with the information of number of unread records. 
> 
> Requesting data by client: 
> When the client calls "GetVoiceHistory" method,plugin uses temp_tail and temp_unread variables to accomplish sending data to client. It copies tail pointer stored in the header into temporary tail pointer ,starts to read history records one at a time from the location pointed by the temp tail pointer , copies the into dbus structure, increments the temp tail pointer by size of the history record (78 bytes) and reads data from the next record location , this is repeated until it reaches head pointer. when all the unread records are read then the dbus structure is sent to the client. Once the data received, client needs to "SetVoiceHistoryRead" method to indicate that it received the data. Plugin will update the tail pointer with the temp tail pointer and unread records number is decremented by temp_unread. Using temp_tail and temp_unread variables protect from modifying the header if there is any loss in data delivery. If client doesnt call 'SetVoiceHistoryRead' then the actua
>  l header pointers wont be updated in the file. 
> 
> Writing record into the file:
> When Ofono calls the plugin with call information (phone number, call type, call start time , call end time), plugin increaments the lastid to create record id for this record ,writes the record in the location pointed by head, and the head pointer is incremented by size of record (78 bytes), "unread" variable is incremented by 1. If the head reaches end of file (3916) then it is reset it back to begining of the data portion of the file (16 bytes). A dbus signal is sent out with number of current unread records. Plugin keeps writing the records in circular queue.

This sounds all way too complicated. And the D-Bus API design is
actually racy since there is no clear access control on the history
information.

I think the proper solution is to create a call history plugin that
creates a D-Bus interface for a call history agent.

That way we know when an agent is present to consume history information
and only if none is present we have to cache it on disk. And once an
agent becomes available flush the cached information.

And the cache should be just be done with a simply appending to a file
and then truncating it after reading it.

Regards

Marcel

RE: Description of Voice call history plugin patch

[Bommaraju, Rajyalakshmi]

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

Please see my comments below.

-----Original Message-----
From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of Marcel Holtmann
Sent: Friday, September 17, 2010 1:17 AM
To: ofono(a)ofono.org
Subject: Re: Description of Voice call history plugin patch
Marcel,

> Callhistory is a plugin loaded by ofono for persisting voice call history information in a disk file. Plugin exports dbus methods and signals as org.ofono.callhistory interface. Whenever there is a voice call in the system, ofono calls the plugin with the infomation (lineid,calltype,start time, end time) , plugin writes the information into the file , increments the 'unread' field in the header and sends out dbus signal with the number of unread records. Client (dialer) requests history records by calling method "GetVoiceHistory" exposed on the callhistory interface. After receiving information client calls the "SetVoiceHistoryRead" as an acknowledgement to indicate end of transaction. The pointers in the header are adjusted appropriately (explained clearly below).
> 
> Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.

I am not buying into this performance argument at all. How many calls to
you expect per second.

Raji >> Even if there are not many calls, operating system takes care of flushing the file to disk if there is any ofono crash. 
 
> When the plugin is loaded by the ofono, it opens or creates a disk file of 3916 bytes size and maps it into fixed size memory space. Plugin reads the header of the file , if the 'unread' records is > 0 , a signal is sent out on the dbus with the information of number of unread records. 
> 
> Requesting data by client: 
> When the client calls "GetVoiceHistory" method,plugin uses temp_tail and temp_unread variables to accomplish sending data to client. It copies tail pointer stored in the header into temporary tail pointer ,starts to read history records one at a time from the location pointed by the temp tail pointer , copies the into dbus structure, increments the temp tail pointer by size of the history record (78 bytes) and reads data from the next record location , this is repeated until it reaches head pointer. when all the unread records are read then the dbus structure is sent to the client. Once the data received, client needs to "SetVoiceHistoryRead" method to indicate that it received the data. Plugin will update the tail pointer with the temp tail pointer and unread records number is decremented by temp_unread. Using temp_tail and temp_unread variables protect from modifying the header if there is any loss in data delivery. If client doesnt call 'SetVoiceHistoryRead' then the act
 ua
>  l header pointers wont be updated in the file. 
> 
> Writing record into the file:
> When Ofono calls the plugin with call information (phone number, call type, call start time , call end time), plugin increaments the lastid to create record id for this record ,writes the record in the location pointed by head, and the head pointer is incremented by size of record (78 bytes), "unread" variable is incremented by 1. If the head reaches end of file (3916) then it is reset it back to begining of the data portion of the file (16 bytes). A dbus signal is sent out with number of current unread records. Plugin keeps writing the records in circular queue.

This sounds all way too complicated. And the D-Bus API design is
actually racy since there is no clear access control on the history
information.

I think the proper solution is to create a call history plugin that
creates a D-Bus interface for a call history agent.

That way we know when an agent is present to consume history information
and only if none is present we have to cache it on disk. And once an
agent becomes available flush the cached information.

And the cache should be just be done with a simply appending to a file
and then truncating it after reading it.

Raji >> Marcel, James Ketrenos discussed this design with you back in april. I will send you the thread on that. I was under impression that you both agreed on this design.

Thanks for feedback,
Raji

_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono

RE: Description of Voice call history plugin patch

[Marcel Holtmann]

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

Hi Raji,

> > Callhistory is a plugin loaded by ofono for persisting voice call history information in a disk file. Plugin exports dbus methods and signals as org.ofono.callhistory interface. Whenever there is a voice call in the system, ofono calls the plugin with the infomation (lineid,calltype,start time, end time) , plugin writes the information into the file , increments the 'unread' field in the header and sends out dbus signal with the number of unread records. Client (dialer) requests history records by calling method "GetVoiceHistory" exposed on the callhistory interface. After receiving information client calls the "SetVoiceHistoryRead" as an acknowledgement to indicate end of transaction. The pointers in the header are adjusted appropriately (explained clearly below).
> > 
> > Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.
> 
> I am not buying into this performance argument at all. How many calls to
> you expect per second.
> 
> Raji >> Even if there are not many calls, operating system takes care of flushing the file to disk if there is any ofono crash. 

what is the difference of calling msync() compared to fdatasync() here.
I highly doubt that we have any issue in either case. A Linux IO and
filesystem expert could answer this, but in the end both of them end in
IO activity. And once you write to it you need to sync to ensure they
are actually written to disk.

This is also not really direkt IO vs memory mapped file. I couldn't care
less in the end. However this is about the complexity that is needed for
maintaining a round-robin file. I am failing to see the need for it.
 
> > When the plugin is loaded by the ofono, it opens or creates a disk file of 3916 bytes size and maps it into fixed size memory space. Plugin reads the header of the file , if the 'unread' records is > 0 , a signal is sent out on the dbus with the information of number of unread records. 
> > 
> > Requesting data by client: 
> > When the client calls "GetVoiceHistory" method,plugin uses temp_tail and temp_unread variables to accomplish sending data to client. It copies tail pointer stored in the header into temporary tail pointer ,starts to read history records one at a time from the location pointed by the temp tail pointer , copies the into dbus structure, increments the temp tail pointer by size of the history record (78 bytes) and reads data from the next record location , this is repeated until it reaches head pointer. when all the unread records are read then the dbus structure is sent to the client. Once the data received, client needs to "SetVoiceHistoryRead" method to indicate that it received the data. Plugin will update the tail pointer with the temp tail pointer and unread records number is decremented by temp_unread. Using temp_tail and temp_unread variables protect from modifying the header if there is any loss in data delivery. If client doesnt call 'SetVoiceHistoryRead' then the act
>  ua
> >  l header pointers wont be updated in the file. 
> > 
> > Writing record into the file:
> > When Ofono calls the plugin with call information (phone number, call type, call start time , call end time), plugin increaments the lastid to create record id for this record ,writes the record in the location pointed by head, and the head pointer is incremented by size of record (78 bytes), "unread" variable is incremented by 1. If the head reaches end of file (3916) then it is reset it back to begining of the data portion of the file (16 bytes). A dbus signal is sent out with number of current unread records. Plugin keeps writing the records in circular queue.
> 
> This sounds all way too complicated. And the D-Bus API design is
> actually racy since there is no clear access control on the history
> information.
> 
> I think the proper solution is to create a call history plugin that
> creates a D-Bus interface for a call history agent.
> 
> That way we know when an agent is present to consume history information
> and only if none is present we have to cache it on disk. And once an
> agent becomes available flush the cached information.
> 
> And the cache should be just be done with a simply appending to a file
> and then truncating it after reading it.
> 
> Raji >> Marcel, James Ketrenos discussed this design with you back in april. I will send you the thread on that. I was under impression that you both agreed on this design.

Just get one thing straight here. Design is design and implementation is
something totally different.

If we use a memory mapped file aside, the whole D-Bus API is pretty much
racy in my eyes since you can not track the history consumer application
use. You don't know when it is running or not. You don't know what to
cache or not. You can not handle crashing of the history consumer
application.

This current implementation is rather complicated to what it is suppose
to achieve. We have two things here, one is the general questions about
on how the D-Bus API of this should actually work. And second about your
current implementation.

The current API has limitations as listed above. These needs to be
addressed and clear way forward seems an agent based model.

The implementation has some flaws. For starters the locking that you
still haven't explained why it is correct. And I agree with Denis here
that I don't see what it is doing. Second is the file system format that
can't work properly on different architectures. You need to use properly
sized variable and ensure the structures memory layout is the same. I
even forgot to mention big endian vs little endian.

Without these fixed, I am not even considering this for upstream.

Regards

Marcel

RE: Description of Voice call history plugin patch

[Bommaraju, Rajyalakshmi]

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

What is the call history agent idea? Can you explain clearly? 

When the client is up, is the history plug-in sends data instead of any signal? How about acknowledgement for data delivery especially when we are sending data cached on disk?

If we forefeit the memory mapped idea, is the plugin writing & reading directly from the file? 

I will send out a simple algorithm once I understand what you are proprosing and once we are on the same page I will implement that and send you the patch. 

Can I send the patch with is developer tested code? Does it need to be tested by QA? 

Thanks & regards,
Raji

-----Original Message-----
From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of Marcel Holtmann
Sent: Friday, September 17, 2010 1:17 AM
To: ofono(a)ofono.org
Subject: Re: Description of Voice call history plugin patch

Hi Raji,

> Callhistory is a plugin loaded by ofono for persisting voice call history information in a disk file. Plugin exports dbus methods and signals as org.ofono.callhistory interface. Whenever there is a voice call in the system, ofono calls the plugin with the infomation (lineid,calltype,start time, end time) , plugin writes the information into the file , increments the 'unread' field in the header and sends out dbus signal with the number of unread records. Client (dialer) requests history records by calling method "GetVoiceHistory" exposed on the callhistory interface. After receiving information client calls the "SetVoiceHistoryRead" as an acknowledgement to indicate end of transaction. The pointers in the header are adjusted appropriately (explained clearly below).
> 
> Design : Plugin uses memory mapped file for high performance input/output operations. File is used as cyclic queue for storing or reading records, it store s 50 records of 78bytes,hence fixed size file of 3916 bytes is used. File's first 16 bytes were used for storing header, and next 3900 bytes for data. Header structure has head pointer, tail pointer,unread and lastid. Head pointer points to next slot for writing record into,tail for reading record, unread for number of history records unread by client, lastid for id of the last record that is written.

I am not buying into this performance argument at all. How many calls to
you expect per second.
 
> When the plugin is loaded by the ofono, it opens or creates a disk file of 3916 bytes size and maps it into fixed size memory space. Plugin reads the header of the file , if the 'unread' records is > 0 , a signal is sent out on the dbus with the information of number of unread records. 
> 
> Requesting data by client: 
> When the client calls "GetVoiceHistory" method,plugin uses temp_tail and temp_unread variables to accomplish sending data to client. It copies tail pointer stored in the header into temporary tail pointer ,starts to read history records one at a time from the location pointed by the temp tail pointer , copies the into dbus structure, increments the temp tail pointer by size of the history record (78 bytes) and reads data from the next record location , this is repeated until it reaches head pointer. when all the unread records are read then the dbus structure is sent to the client. Once the data received, client needs to "SetVoiceHistoryRead" method to indicate that it received the data. Plugin will update the tail pointer with the temp tail pointer and unread records number is decremented by temp_unread. Using temp_tail and temp_unread variables protect from modifying the header if there is any loss in data delivery. If client doesnt call 'SetVoiceHistoryRead' then the act
 ua
>  l header pointers wont be updated in the file. 
> 
> Writing record into the file:
> When Ofono calls the plugin with call information (phone number, call type, call start time , call end time), plugin increaments the lastid to create record id for this record ,writes the record in the location pointed by head, and the head pointer is incremented by size of record (78 bytes), "unread" variable is incremented by 1. If the head reaches end of file (3916) then it is reset it back to begining of the data portion of the file (16 bytes). A dbus signal is sent out with number of current unread records. Plugin keeps writing the records in circular queue.

This sounds all way too complicated. And the D-Bus API design is
actually racy since there is no clear access control on the history
information.

I think the proper solution is to create a call history plugin that
creates a D-Bus interface for a call history agent.

That way we know when an agent is present to consume history information
and only if none is present we have to cache it on disk. And once an
agent becomes available flush the cached information.

And the cache should be just be done with a simply appending to a file
and then truncating it after reading it.

Raji > What is the call history agent idea? Can you explain clearly? 

When the client is up, is the history plug-in sends data instead of any signal? How about acknowledgement for data delivery especially when we are sending data cached on disk?

If we forefeit the memory mapped idea, is the plugin writing & reading directly from the file? 

I will send out a simple algorithm once I understand what you are proprosing and once we are on the same page I will implement that and send you the patch. 

Can I send the patch with is developer tested code? Does it need to be tested by QA? 

Thanks & regards,
Raji
_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono

RE: Description of Voice call history plugin patch

[Marcel Holtmann]

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

Hi Raji,

just a friendly reminder here to not do top posting. We are not allowing
this on an open source mailing list. It is a good way of actually
getting ignored ;)

> What is the call history agent idea? Can you explain clearly? 

It is like the SIM Toolkit agent, the PIN agent in BlueZ. Read the
documentation and the examples. It is really simple.

In the end an agent is a callback interface inside the UI or an
application outside of oFono. It is were oFono asks for further
information or were it relays information to. In this case the call
history information.

The agent design has the advantage the oFono knows when an agent is
present and when not. So until no agent is present it can spool the
history to disk. And once an agent becomes available replay it to the
agent.

> When the client is up, is the history plug-in sends data instead of any signal? How about acknowledgement for data delivery especially when we are sending data cached on disk?

The agent are like callbacks. They are D-Bus method calls. The
acknowledgment is implicit here with the method return or error in case
of problems.

> If we forefeit the memory mapped idea, is the plugin writing & reading directly from the file? 

I personally would just write to the file. However if you wanna do
memory mapped file in round-robin fashion, then that works as well.
However in case of round-robin files you need to think about the memory
layout of your file.

> I will send out a simple algorithm once I understand what you are proprosing and once we are on the same page I will implement that and send you the patch. 

Send a D-Bus API document first. See the SIM Toolkit API for an example.

> Can I send the patch with is developer tested code? Does it need to be tested by QA? 

You can even send non-working or non-complete code for review at any
time. Just mark them as RFC (request for comments).

Regards

Marcel