RE: Description of Voice call history plugin patch

[Marcel Holtmann <marcel@holtmann.org>]

[-- 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