Patch for voice call history plugin merge

Hi,

I have implemented voice call history plugin using memory mapped file ,
this implementation tested on meego images right now as an external
plugin to ofono. I am submitting the code here so that it can be merged and will be used as builtin plugin of ofono in future. Please review the patch and let me know your decision and any changes that needs to be done for accepting it.

Thanks
Raji Bommaraju

From: Raji Bommaraju <Rajyalakshmi Bommaraju>
Date: Tue, 18 May 2010 09:53:09 -0700
Subject: [PATCH] Merging voice call history plugin implementation

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

index 7fd862f..d2f1aa1 100644
@@ -239,6 +239,8 @@ builtin_sources += plugins/palmpre.c
 builtin_modules += ste
 builtin_sources += plugins/ste.c
 
+builtin_modules += callhistory 
+builtin_sources += plugins/callhistory.c
 endif
 
 if MAINTAINER_MODE

1.6.6.1

Hi Raji,

A few general comments:
- Please submit your patches using git send-email so that they can be commented on inline. This ensures that many other eyes will look at your patches.
- Before submitting your patches, make sure to run checkpatch.pl from the latest kernel to check for style issues. Running checkpatch.pl locally gives me over 100 errors with your submission.
- You have to give a much better explanation for what exactly you're doing with the code. For instance, why are semaphores used to protect writing of the call history file? Who are you protecting from? Give an overview of at least several sized paragraphs, the more the better. Assume that nobody has an idea about what you're trying to accomplish and you have to educate them.

Other style issues to watch out for:
+// Global Shared data
+typedef struct _SHARED
+{
+ HistoryFileHeader header;
+ void *dataMap;
+ sem_t mutex;
+ int temp_unread;
+ int temp_tail;
+} SharedData;
+

oFono does not use CamelCase for structures, simply use struct file_header {} or similar. We also don't use // style comments.

+static SharedData *shared_data = NULL;
+
+

Double empty lines are a big no no, get rid of them.

+static void callhistory_emit_voice_history_changed(int ); +

forward declarations should be avoided, particularly for static functions

+ if (unread > 0){
+ callhistory_emit_voice_history_changed(unread);
+ }

Braces for single-statement if/while/for blocks are not to be used

+ if (!fill ){
+ ofono_debug("Error allocating init memory");
+ return FALSE;
+ }
+

These lines contain mixed tab / space indentation. Only tabs are to be used for indentation.

The if statement should be preceded and followed by an empty line.

+ // create semaphore
+ if (init_sem() < 0)
+ return FALSE;
+

The return statement contains a space@the end. All maintainers have git settings that will complain loudly when we try to apply such patches. There are many more occurrences of this and all of them need to be fixed.

Add:

[apply]
whitespace = error

to your gitconfig and make sure your patch applies cleanly before submitting.

+ if (unread > 0)
+ callhistory_emit_voice_history_changed(unread);
+
+}

Empty lines at the end of blocks are not required

For other style issues, if in doubt check existing code, it is quite consistent. In general the code won't be looked at in detail until it conforms to the style guide.

Please fixup all these issues and resubmit.

Regards,
-Denis