Re: Patch for voice call history plugin merge

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Raji,

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

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