[Opensync-devel] [PATCH] API change to osync_time_utcoffset2sec()

[Chris F. <cd...@fo...>]

Hi,

I noticed there was some error handling cleanup done in opensync_time.c
so I took the time to clarify the documentation on the return codes.
I commited one patch already which was fairly innocuous.

The following patch changes the API, so I post it here for review.

- Chris



Fixed osync_time_utcoffset2sec()'s mistaken reliance on sscanf()

Tightened up checking for input format validation, according to
the documented comments.

Added an OSyncError parameter, for the error case.

Added a test for osync_time_utcoffset2sec()
---
 opensync/format/opensync_time.c |   23 ++++++++++++++++++-----
 opensync/format/opensync_time.h |    6 ++++--
 tests/format-tests/check_time.c |   36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/opensync/format/opensync_time.c b/opensync/format/opensync_time.c
index 5f1ac9e..8a5939a 100644
--- a/opensync/format/opensync_time.c
+++ b/opensync/format/opensync_time.c
@@ -21,6 +21,7 @@
  */
 
 #include <time.h>
+#include <ctype.h>
 
 #include "opensync.h"
 #include "opensync_time.h"
@@ -627,19 +628,31 @@ error:
 	return NULL;
 }
 
-int osync_time_utcoffset2sec(const char *offset)
+int osync_time_utcoffset2sec(const char *offset, OSyncError **error)
 {
 	char csign = 0;
 	int seconds = 0, sign = 1;
 	int hours = 0, minutes = 0;
 	osync_trace(TRACE_ENTRY, "%s(%s)", __func__, offset);
 
-	sscanf(offset, "%c%2d%2d", &csign, &hours, &minutes);
+	// make sure the format of offset is what we expect: [-+][0-9]{4}
+	if( strlen(offset) >= 5 &&
+	    (offset[0] == '-' || offset[0] == '+') &&
+	    isdigit(offset[1]) && isdigit(offset[2]) &&
+	    isdigit(offset[3]) && isdigit(offset[4]) &&
+	    sscanf(offset, "%c%2d%2d", &csign, &hours, &minutes) == 3 )
+	{
 
-	if (csign == '-')
-		sign = -1;
+		if (csign == '-')
+			sign = -1;
 
-	seconds = (hours * 3600 + minutes * 60) * sign; 
+		seconds = (hours * 3600 + minutes * 60) * sign; 
+
+	}
+	else {
+		osync_error_set(error, OSYNC_ERROR_GENERIC, "%s: unable to parse utc offset into seconds: %s", __func__, offset);
+		osync_trace(TRACE_INTERNAL, "%s: unable to parse utc offset into seconds: %s", __func__, offset);
+	}
 
 	osync_trace(TRACE_EXIT, "%s: %i", __func__, seconds);
 	return seconds;
diff --git a/opensync/format/opensync_time.h b/opensync/format/opensync_time.h
index a7cebbb..cc237f8 100644
--- a/opensync/format/opensync_time.h
+++ b/opensync/format/opensync_time.h
@@ -263,9 +263,11 @@ OSYNC_EXPORT char *osync_time_vtime2localtime(const char* utc, int offset, OSync
 /** @brief Function converts UTC offset string in offset in seconds
  *
  * @param offset The offset string of the form a timezone field (Example +0200) 
- * @returns seconds of UTC offset 
+ * @error An OSyncError struct
+ * @returns seconds of UTC offset. On error, osync_error_is_set(error) is
+ *	true, and the return value is 0.
  */ 
-OSYNC_EXPORT int osync_time_utcoffset2sec(const char *offset);
+OSYNC_EXPORT int osync_time_utcoffset2sec(const char *offset, OSyncError **error);
 
 /*@}*/
 
diff --git a/tests/format-tests/check_time.c b/tests/format-tests/check_time.c
index 1cccf82..bc30cc3 100644
--- a/tests/format-tests/check_time.c
+++ b/tests/format-tests/check_time.c
@@ -211,9 +211,45 @@ START_TEST (time_unix_converters)
 }
 END_TEST
 
+START_TEST (time_utc_offset)
+{
+	OSyncError *error = NULL;
+	int seconds;
+
+	// UTC offset format is %c%2d%2d, i.e. +0200
+
+	// test missing lead char
+	osync_time_utcoffset2sec("0400", &error);
+	fail_unless(osync_error_is_set(&error), NULL);
+	osync_error_unref(&error);
+	error = NULL;
+
+	// test missing numbers
+	osync_time_utcoffset2sec("-040", &error);
+	fail_unless(osync_error_is_set(&error), NULL);
+	osync_error_unref(&error);
+	error = NULL;
+
+	// test correct operation [1]
+	seconds = osync_time_utcoffset2sec("-0400", &error);
+	fail_unless(!osync_error_is_set(&error), NULL);
+	fail_unless(seconds == -(4 * 60 * 60), NULL);
+	osync_error_unref(&error);
+	error = NULL;
+
+	// test correct operation, with trailing chars [2]
+	seconds = osync_time_utcoffset2sec("-0400555", &error);
+	fail_unless(!osync_error_is_set(&error), NULL);
+	fail_unless(seconds == -(4 * 60 * 60), NULL);
+	osync_error_unref(&error);
+	error = NULL;
+}
+END_TEST
+
 OSYNC_TESTCASE_START("time")
 OSYNC_TESTCASE_ADD(time_timezone_diff)
 OSYNC_TESTCASE_ADD(time_relative2tm)
 OSYNC_TESTCASE_ADD(time_unix_converters)
+OSYNC_TESTCASE_ADD(time_utc_offset)
 OSYNC_TESTCASE_END
 
-- 
1.6.2.5