Created attachment 247254 [details] [review]
Setting default log handler and specific debug handler which stores debug messages and printerr handler which prints it out in case of error

Patch provides:
* gdata_test_debug_output function which takes messages in GList, detects XML output and using libxml2 gets it in pretty print format
* gdata_test_debug_handler function which handles debug messages storing them in GSList
* assert_handler function which handles error messages issued by printerr, which in turn is used in case if g_assert fails - it calls gdata_test_debug_output

Review of attachment 247254 [details] [review]:

This looks good; the comments below are just cosmetic, really.

::: gdata/gdata-service.c
@@ +197,2 @@
 	/* Debug log handling */
+	g_log_set_default_handler ((GLogFunc) debug_handler, self);

This should be in a separate git commit, since it’s a self-contained change (because calling g_log_set_handler() is completely wrong in any case).

::: gdata/tests/common.c
@@ +34,3 @@
 
+/* declaration of debug handler */
+static void gdata_test_debug_handler (const char *log_domain, GLogLevelFlags log_level, const char *message, gpointer user_data);

Please use ‘gchar’ instead of ‘char’ for consistency.

@@ +37,3 @@
+
+/* declaration of printerr handler */
+void assert_handler (const char *message);

This should be ‘static’. Use ‘gchar’ instead of ‘char’ for consistency.

@@ +40,3 @@
+
+/* global list of debugging messages */
+GSList *message_list = NULL;

This should also be ‘static’.

@@ +88,3 @@
+	
+	/* Set handler for printerr */
+	GPrintFunc old_func = g_set_printerr_handler ((GPrintFunc) assert_handler);

Please declare all variables (such as old_func) at the top of the function, rather than inline. In any case, there isn’t any point in storing the old handler if you never restore it.

@@ +671,3 @@
+{	
+	if (message_list == NULL) {
+		printf ("No debugging messages.\n");

What’s the rationale for printing a message in this case? I wonder if it would be tidier to not print anything if (message_list == NULL).

@@ +675,3 @@
+	}
+	do {
+	    char *message = (char*) message_list->data;

The indentation’s a little odd here. Also, please use ‘gchar’.

@@ +676,3 @@
+	do {
+	    char *message = (char*) message_list->data;
+		if (message[2] == '<') {

Why access index 2? If (message == NULL) or (strlen(message)<2), this will access undefined memory.

This block needs a brief comment explaining what it does and how it checks for the existence of XML.

@@ +681,3 @@
+			/* create xml document and dump it to string buffer */
+			xmlChar *xml_buff;
+			int buffer_size;

Declare all variables at the head of the block.

@@ +690,3 @@
+		} else {
+		    printf ("%s\n", (char*) message);
+		}

The indentation’s a little wrong here too.

@@ +694,3 @@
+	} while (message_list != NULL);
+	/* finished debug output, clean up GSList */
+	g_slist_free_full (message_list, free);

You should use g_free() instead of free() here, because the memory was allocated with g_strdup() (which could potentially be using a memory allocator which isn’t the standard malloc()/free()).

This g_list_free_full() can be rolled into the big do{}while loop above, by calling g_free() and g_slist_delete_link() on each element within the loop.

@@ +700,3 @@
+gdata_test_debug_handler (const char *log_domain, GLogLevelFlags log_level, const char *message, gpointer user_data)
+{	
+	message_list = g_slist_append (message_list, g_strdup (message));

This could do with a comment explaining that it’s storing the log message for later playback.

@@ +703,3 @@
+}
+
+void assert_handler (const char *message)

This should be static, and the ‘static void’ should be on a different line from ‘assert_handler’. Again, please use ‘gchar’ rather than ‘char’. :-)

::: gdata/tests/common.h
@@ +38,2 @@
 /* These two must not match (obviously) */
+#define PASSWORD "KrishAsa"

These two changes shouldn’t be committed. :-)

Created attachment 247283 [details] [review]
Change g_log_set_handler to g_log_set_default_handler

Change g_log_set_handler to g_log_set_default_handler in gdata/gdata-service.c so we could set handler for debug log level in gdata/tests/common.c.

Created attachment 247284 [details] [review]
Added functions gdata_test_debug_output, gdata_test_debug_handler and GSList message_list

Added functions gdata_test_debug_output, gdata_test_debug_handler and GSList message_list for collecting debug messages in memory and printing out only when needed (including pretty printing XML).

Review of attachment 247283 [details] [review]:

Looks good. Please commit to git master!

Please mention this bug URI in the commit message, e.g. with a line “Helps: https://bugzilla.gnome.org/show_bug.cgi?id=627895”.

Review of attachment 247284 [details] [review]:

A few more stylistic points. Please commit once these are fixed, remembering to put “Closes: https://bugzilla.gnome.org/show_bug.cgi?id=627895” in the commit message. Thanks!

::: gdata/tests/common.c
@@ +678,3 @@
+		gchar *message = (gchar*) message_list->data;
+		if (message != NULL) {
+			if (strlen(message)>2 && message[2] == '<') {

Please put a space before the ‘(’ in the strlen() call, and spaces around the ‘>’ operator.

@@ +681,3 @@
+				// As debug string starts with direction indicator and space, t.i. "< ",
+				// we need access string starting from third character and see if it's 
+				// looks like xml - t.i. it starts with '<'

Please use block-style comments (/* like this */) instead of single-line comments.

@@ +702,3 @@
+		}
+		/* free GSList node */
+	    message_list = g_slist_delete_link(message_list, message_list);

Indentation problems here. Also, please put a space before the ‘(’ of a function call.

::: gdata/tests/common.h
@@ +286,3 @@
 
+/* Debugging output in case of assert failure */
+static void gdata_test_debug_output (void);

This should not be ‘static’. A static function is scoped to its translation unit, which in this case would mean it might not be callable from other test code.

Good stuff! Thanks for committing them both.

commit b116aade27c2a4b4229ba0b6cca4a425f9dfcb38
Author: Peteris Krisjanis <pecisk@gmail.com>
Date:   Fri Jun 21 18:55:51 2013 +0300

    Added functions gdata_test_debug_output, gdata_test_debug_handler and GSList

 gdata/tests/common.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdata/tests/common.h |  3 +++
 2 files changed, 70 insertions(+)

commit 59aea63e87277fe7d320add954403105be3f0405
Author: Peteris Krisjanis <pecisk@gmail.com>
Date:   Fri Jun 21 18:35:45 2013 +0300

    Change g_log_set_handler to g_log_set_default_handler so we could set handle

 gdata/gdata-service.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)