[PATCH 1/5] string: Add l_string_unwrap and simplify l_string_free

[PATCH 1/5] string: Add l_string_unwrap and simplify l_string_free

[Mat Martineau]

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

This removes the "free a string object but return the internal data"
functionality of l_string_free. The option to not free everything was a
source of confusion.

l_string_unwrap is the same as the old l_string_free(ptr, false).
---
 ell/dbus-filter.c   |  4 ++--
 ell/dbus-service.c  |  2 +-
 ell/dbus-util.c     |  4 ++--
 ell/gvariant-util.c |  4 ++--
 ell/key.c           |  2 +-
 ell/settings.c      |  2 +-
 ell/string.c        | 32 +++++++++++++++++++++-----------
 ell/string.h        |  3 ++-
 8 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/ell/dbus-filter.c b/ell/dbus-filter.c
index 9e3cad5..0af1b67 100644
--- a/ell/dbus-filter.c
+++ b/ell/dbus-filter.c
@@ -393,7 +393,7 @@ char *_dbus_filter_rule_to_str(const struct _dbus_filter_condition *rule,
 					rule->type - L_DBUS_MATCH_ARG0);
 			break;
 		default:
-			l_string_free(str, true);
+			l_string_free(str);
 			return NULL;
 		}
 
@@ -417,5 +417,5 @@ char *_dbus_filter_rule_to_str(const struct _dbus_filter_condition *rule,
 			l_string_append_c(str, ',');
 	}
 
-	return l_string_free(str, false);
+	return l_string_unwrap(str);
 }
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 98f6e7a..8bc9244 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -1600,7 +1600,7 @@ bool _dbus_object_tree_dispatch(struct _dbus_object_tree *tree,
 
 		buf = l_string_new(0);
 		_dbus_object_tree_introspect(tree, path, buf);
-		xml = l_string_free(buf, false);
+		xml = l_string_unwrap(buf);
 
 		reply = l_dbus_message_new_method_return(message);
 		l_dbus_message_set_arguments(reply, "s", xml);
diff --git a/ell/dbus-util.c b/ell/dbus-util.c
index 1298228..c4e8419 100644
--- a/ell/dbus-util.c
+++ b/ell/dbus-util.c
@@ -901,7 +901,7 @@ void _dbus1_builder_free(struct dbus_builder *builder)
 	if (unlikely(!builder))
 		return;
 
-	l_string_free(builder->signature, true);
+	l_string_free(builder->signature);
 	l_queue_destroy(builder->containers,
 				(l_queue_destroy_func_t) container_free);
 	l_free(builder->body);
@@ -1265,7 +1265,7 @@ char *_dbus1_builder_finish(struct dbus_builder *builder,
 	if (unlikely(l_queue_length(builder->containers) != 1))
 		return NULL;
 
-	signature = l_string_free(builder->signature, false);
+	signature = l_string_unwrap(builder->signature);
 	builder->signature = NULL;
 
 	*body = builder->body;
diff --git a/ell/gvariant-util.c b/ell/gvariant-util.c
index 2172ad9..e6f70ad 100644
--- a/ell/gvariant-util.c
+++ b/ell/gvariant-util.c
@@ -918,7 +918,7 @@ void _gvariant_builder_free(struct dbus_builder *builder)
 	if (unlikely(!builder))
 		return;
 
-	l_string_free(builder->signature, true);
+	l_string_free(builder->signature);
 	l_queue_destroy(builder->containers,
 				(l_queue_destroy_func_t) container_free);
 	l_free(builder->body);
@@ -1319,7 +1319,7 @@ char *_gvariant_builder_finish(struct dbus_builder *builder,
 
 	root = l_queue_peek_head(builder->containers);
 
-	signature = l_string_free(builder->signature, false);
+	signature = l_string_unwrap(builder->signature);
 	builder->signature = NULL;
 
 	if (_gvariant_is_fixed_size(signature)) {
diff --git a/ell/key.c b/ell/key.c
index 370b3c8..11700e8 100644
--- a/ell/key.c
+++ b/ell/key.c
@@ -169,7 +169,7 @@ static char *format_key_info(const char *encoding, const char *hash)
 	if (hash)
 		l_string_append_printf(info, "hash=%s", hash);
 
-	return l_string_free(info, false);
+	return l_string_unwrap(info);
 }
 
 static long kernel_query_key(int32_t key_serial, const char *encoding,
diff --git a/ell/settings.c b/ell/settings.c
index c6eec35..08ba967 100644
--- a/ell/settings.c
+++ b/ell/settings.c
@@ -427,7 +427,7 @@ LIB_EXPORT char *l_settings_to_data(struct l_settings *settings, size_t *len)
 		group_entry = group_entry->next;
 	}
 
-	ret = l_string_free(buf, false);
+	ret = l_string_unwrap(buf);
 
 	if (len)
 		*len = strlen(ret);
diff --git a/ell/string.c b/ell/string.c
index 98e1c46..e6e7140 100644
--- a/ell/string.c
+++ b/ell/string.c
@@ -117,27 +117,37 @@ LIB_EXPORT struct l_string *l_string_new(size_t initial_length)
 /**
  * l_string_free:
  * @string: growable string object
+ *
+ * Free the growable string object and all associated data
+ **/
+LIB_EXPORT void l_string_free(struct l_string *string)
+{
+	if (unlikely(!string))
+		return;
+
+	l_free(string->str);
+	l_free(string);
+}
+
+/**
+ * l_string_unwrap:
+ * @string: growable string object
  * @free_data: internal string array
  *
- * Free the growable string object.  If @free_data #true, then the internal
- * string data will be freed and NULL will be returned.  Otherwise the
- * internal string data will be returned to the caller.  The caller is
- * responsible for freeing it using l_free().
+ * Free the growable string object and return the internal string data.
+ * The caller is responsible for freeing the string data using l_free(),
+ * and the string object is no longer usable.
  *
- * Returns: @string's internal buffer or NULL
+ * Returns: @string's internal buffer
  **/
-LIB_EXPORT char *l_string_free(struct l_string *string, bool free_data)
+LIB_EXPORT char *l_string_unwrap(struct l_string *string)
 {
 	char *result;
 
 	if (unlikely(!string))
 		return NULL;
 
-	if (free_data) {
-		l_free(string->str);
-		result = NULL;
-	} else
-		result = string->str;
+	result = string->str;
 
 	l_free(string);
 
diff --git a/ell/string.h b/ell/string.h
index a778b31..6549a05 100644
--- a/ell/string.h
+++ b/ell/string.h
@@ -100,7 +100,8 @@ static inline bool __attribute__ ((always_inline))
 }
 
 struct l_string *l_string_new(size_t initial_length);
-char *l_string_free(struct l_string *string, bool free_data);
+void l_string_free(struct l_string *string);
+char *l_string_unwrap(struct l_string *string);
 
 struct l_string *l_string_append(struct l_string *dest, const char *src);
 struct l_string *l_string_append_c(struct l_string *dest, const char c);
-- 
2.10.2

[PATCH 2/5] unit: Update for l_string_free changes

[Mat Martineau]

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

---
 unit/test-dbus-service.c | 10 +++++-----
 unit/test-string.c       | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/unit/test-dbus-service.c b/unit/test-dbus-service.c
index c27869b..269e0e6 100644
--- a/unit/test-dbus-service.c
+++ b/unit/test-dbus-service.c
@@ -116,7 +116,7 @@ static void test_introspect_method(const void *test_data)
 
 	buf = l_string_new(0);
 	_dbus_method_introspection(method, buf);
-	xml = l_string_free(buf, false);
+	xml = l_string_unwrap(buf);
 
 	assert(!strcmp(test->expected_xml, xml));
 	l_free(xml);
@@ -134,7 +134,7 @@ static void test_introspect_signal(const void *test_data)
 
 	buf = l_string_new(0);
 	_dbus_signal_introspection(signal, buf);
-	xml = l_string_free(buf, false);
+	xml = l_string_unwrap(buf);
 
 	assert(!strcmp(test->expected_xml, xml));
 	l_free(xml);
@@ -152,7 +152,7 @@ static void test_introspect_property(const void *test_data)
 
 	buf = l_string_new(0);
 	_dbus_property_introspection(property, buf);
-	xml = l_string_free(buf, false);
+	xml = l_string_unwrap(buf);
 
 	assert(!strcmp(test->expected_xml, xml));
 	l_free(xml);
@@ -166,7 +166,7 @@ static void test_introspect_interface(const void *test_data)
 
 	buf = l_string_new(0);
 	_dbus_interface_introspection(interface, buf);
-	xml = l_string_free(buf, false);
+	xml = l_string_unwrap(buf);
 
 	assert(!strcmp(test->expected_xml, xml));
 	l_free(xml);
@@ -358,7 +358,7 @@ static void test_dbus_object_tree_introspection(const void *test_data)
 
 	buf = l_string_new(1024);
 	_dbus_object_tree_introspect(tree, "/", buf);
-	xml = l_string_free(buf, false);
+	xml = l_string_unwrap(buf);
 	assert(!strcmp(ofono_manager_introspection, xml));
 	l_free(xml);
 
diff --git a/unit/test-string.c b/unit/test-string.c
index 4c6a38c..d5b0d9d 100644
--- a/unit/test-string.c
+++ b/unit/test-string.c
@@ -40,7 +40,7 @@ static void test_grow(const void *test_data)
 	assert(l_string_append(str, "BarFoo"));
 	assert(l_string_length(str) == strlen("Foobar7BarFoo"));
 
-	a = l_string_free(str, false);
+	a = l_string_unwrap(str);
 	assert(a);
 	assert(!strcmp(a, "Foobar7BarFoo"));
 
@@ -60,7 +60,7 @@ static void test_printf(const void *test_data)
 
 	assert(l_string_length(str) == strlen("Foobar7100BarFoo"));
 
-	a = l_string_free(str, false);
+	a = l_string_unwrap(str);
 	assert(a);
 	assert(!strcmp(a, "Foobar7100BarFoo"));
 
@@ -107,7 +107,7 @@ static void test_fixed(const void *test_data)
 	l_string_append_fixed(str, test->input, test->input_len);
 	assert(l_string_length(str) == strlen(test->expected));
 
-	a = l_string_free(str, false);
+	a = l_string_unwrap(str);
 	assert(a);
 	assert(!strcmp(a, test->expected));
 
@@ -125,7 +125,7 @@ static void test_truncate(const void *test_data)
 	assert(!l_string_truncate(NULL, 8));
 
 	assert(l_string_truncate(str, 7));
-	a = l_string_free(str, false);
+	a = l_string_unwrap(str);
 	assert(a);
 	assert(!strcmp(a, "Foobar7"));
 	l_free(a);
@@ -134,7 +134,7 @@ static void test_truncate(const void *test_data)
 	l_string_append(str, "Foobar7");
 	assert(l_string_truncate(str, 3));
 	l_string_append_c(str, '4');
-	a = l_string_free(str, false);
+	a = l_string_unwrap(str);
 	assert(a);
 	assert(!strcmp(a, "Foo4"));
 	l_free(a);
-- 
2.10.2

[PATCH 3/5] timeout: Add millisecond timeout functions, remove nanosecond functions

[Mat Martineau]

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

Nanosecond resolution was overkill, but there are still common use cases
for millisecond timer resolution.
---
 ell/timeout.c | 109 +++++++++++++++++++++++++++++++++++++++++-----------------
 ell/timeout.h |   8 ++---
 2 files changed, 82 insertions(+), 35 deletions(-)

diff --git a/ell/timeout.c b/ell/timeout.c
index fc24d60..0935fd3 100644
--- a/ell/timeout.c
+++ b/ell/timeout.c
@@ -29,6 +29,7 @@
 #include <string.h>
 #include <sys/epoll.h>
 #include <sys/timerfd.h>
+#include <limits.h>
 
 #include "util.h"
 #include "timeout.h"
@@ -91,31 +92,22 @@ static inline int timeout_set(int fd, unsigned int seconds, long nanoseconds)
 	return timerfd_settime(fd, 0, &itimer, NULL);
 }
 
-/**
- * l_timeout_create:
- * @seconds: timeout in seconds
- * @callback: timeout callback function
- * @user_data: user data provided to timeout callback function
- * @destroy: destroy function for user data
- *
- * Create new timeout callback handling.
- *
- * The timeout will on fire once. The timeout handling needs to be rearmed
- * with l_timeout_modify() to trigger again.
- *
- * Returns: a newly allocated #l_timeout object. On failure, the function
- * returns NULL.
- **/
-LIB_EXPORT struct l_timeout *l_timeout_create(unsigned int seconds,
-			l_timeout_notify_cb_t callback,
-			void *user_data, l_timeout_destroy_cb_t destroy)
+static bool convert_ms(unsigned long milliseconds, unsigned int *seconds,
+			long *nanoseconds)
 {
-	return l_timeout_create_with_nanoseconds(seconds, 0, callback,
-							user_data, destroy);
+	unsigned long big_seconds = milliseconds / 1000;
+
+	if (big_seconds > UINT_MAX)
+		return false;
+
+	*seconds = big_seconds;
+	*nanoseconds = (milliseconds % 1000) * 1000000L;
+
+	return true;
 }
 
 /**
- * l_timeout_create_with_nanoseconds:
+ * timeout_create_with_nanoseconds:
  * @seconds: number of seconds
  * @nanoseconds: number of nanoseconds
  * @callback: timeout callback function
@@ -124,13 +116,13 @@ LIB_EXPORT struct l_timeout *l_timeout_create(unsigned int seconds,
  *
  * Create new timeout callback handling.
  *
- * The timeout will on fire once. The timeout handling needs to be rearmed
- * with l_timeout_modify_with_nanoseconds() to trigger again.
+ * The timeout will only fire once. The timeout handling needs to be rearmed
+ * with one of the l_timeout_modify functions to trigger again.
  *
  * Returns: a newly allocated #l_timeout object. On failure, the function
  * returns NULL.
  **/
-LIB_EXPORT struct l_timeout *l_timeout_create_with_nanoseconds(unsigned int seconds,
+static struct l_timeout *timeout_create_with_nanoseconds(unsigned int seconds,
 			long nanoseconds, l_timeout_notify_cb_t callback,
 			void *user_data, l_timeout_destroy_cb_t destroy)
 {
@@ -173,6 +165,58 @@ LIB_EXPORT struct l_timeout *l_timeout_create_with_nanoseconds(unsigned int seco
 }
 
 /**
+ * l_timeout_create:
+ * @seconds: timeout in seconds
+ * @callback: timeout callback function
+ * @user_data: user data provided to timeout callback function
+ * @destroy: destroy function for user data
+ *
+ * Create new timeout callback handling.
+ *
+ * The timeout will only fire once. The timeout handling needs to be rearmed
+ * with one of the l_timeout_modify functions to trigger again.
+ *
+ * Returns: a newly allocated #l_timeout object. On failure, the function
+ * returns NULL.
+ **/
+LIB_EXPORT struct l_timeout *l_timeout_create(unsigned int seconds,
+			l_timeout_notify_cb_t callback,
+			void *user_data, l_timeout_destroy_cb_t destroy)
+{
+	return timeout_create_with_nanoseconds(seconds, 0, callback,
+							user_data, destroy);
+}
+
+/**
+ * l_timeout_create_ms:
+ * @milliseconds: timeout in milliseconds
+ * @callback: timeout callback function
+ * @user_data: user data provided to timeout callback function
+ * @destroy: destroy function for user data
+ *
+ * Create new timeout callback handling.
+ *
+ * The timeout will only fire once. The timeout handling needs to be rearmed
+ * with one of the l_timeout_modify functions to trigger again.
+ *
+ * Returns: a newly allocated #l_timeout object. On failure, the function
+ * returns NULL.
+ **/
+LIB_EXPORT struct l_timeout *l_timeout_create_ms(unsigned long milliseconds,
+			l_timeout_notify_cb_t callback,
+			void *user_data, l_timeout_destroy_cb_t destroy)
+{
+	unsigned int seconds;
+	long nanoseconds;
+
+	if (!convert_ms(milliseconds, &seconds, &nanoseconds))
+		return NULL;
+
+	return timeout_create_with_nanoseconds(seconds, nanoseconds, callback,
+						user_data, destroy);
+}
+
+/**
  * l_timeout_modify:
  * @timeout: timeout object
  * @seconds: timeout in seconds
@@ -197,15 +241,14 @@ LIB_EXPORT void l_timeout_modify(struct l_timeout *timeout,
 }
 
 /**
- * l_timeout_modify_with_nanoseconds:
+ * l_timeout_modify_ms:
  * @timeout: timeout object
- * @seconds: number of seconds
- * @nanoseconds: number of nanoseconds
+ * @milliseconds: number of milliseconds
  *
  * Modify an existing @timeout and rearm it.
  **/
-LIB_EXPORT void l_timeout_modify_with_nanoseconds(struct l_timeout *timeout,
-					unsigned int seconds, long nanoseconds)
+LIB_EXPORT void l_timeout_modify_ms(struct l_timeout *timeout,
+					unsigned long milliseconds)
 {
 	if (unlikely(!timeout))
 		return;
@@ -213,8 +256,12 @@ LIB_EXPORT void l_timeout_modify_with_nanoseconds(struct l_timeout *timeout,
 	if (unlikely(timeout->fd < 0))
 		return;
 
-	if (seconds > 0 || nanoseconds > 0) {
-		if (timeout_set(timeout->fd, seconds, nanoseconds) < 0)
+	if (milliseconds > 0) {
+		unsigned int sec;
+		long nanosec;
+
+		if (!convert_ms(milliseconds, &sec, &nanosec) ||
+			timeout_set(timeout->fd, sec, nanosec) < 0)
 			return;
 	}
 
diff --git a/ell/timeout.h b/ell/timeout.h
index 882f166..288f0ca 100644
--- a/ell/timeout.h
+++ b/ell/timeout.h
@@ -36,13 +36,13 @@ typedef void (*l_timeout_destroy_cb_t) (void *user_data);
 struct l_timeout *l_timeout_create(unsigned int seconds,
 			l_timeout_notify_cb_t callback,
 			void *user_data, l_timeout_destroy_cb_t destroy);
-struct l_timeout *l_timeout_create_with_nanoseconds(unsigned int seconds,
-			long nanoseconds, l_timeout_notify_cb_t callback,
+struct l_timeout *l_timeout_create_ms(unsigned long milliseconds,
+			l_timeout_notify_cb_t callback,
 			void *user_data, l_timeout_destroy_cb_t destroy);
 void l_timeout_modify(struct l_timeout *timeout,
 				unsigned int seconds);
-void l_timeout_modify_with_nanoseconds(struct l_timeout *timeout,
-				unsigned int seconds, long nanoseconds);
+void l_timeout_modify_ms(struct l_timeout *timeout,
+				unsigned long milliseconds);
 void l_timeout_remove(struct l_timeout *timeout);
 
 #ifdef __cplusplus
-- 
2.10.2

[PATCH 4/5] unit: Switch from nanosecond timer to millisecond timer

[Mat Martineau]

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

---
 unit/test-main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/unit/test-main.c b/unit/test-main.c
index 1f4bb31..bd68599 100644
--- a/unit/test-main.c
+++ b/unit/test-main.c
@@ -103,10 +103,8 @@ int main(int argc, char *argv[])
 	timeout_quit = l_timeout_create(3, timeout_quit_handler, NULL, NULL);
 
 	race_delay = l_timeout_create(1, race_delay_handler, NULL, NULL);
-	race1 = l_timeout_create_with_nanoseconds(1, 100000000, race_handler,
-						  &race2, NULL);
-	race2 = l_timeout_create_with_nanoseconds(1, 100000000, race_handler,
-						  &race1, NULL);
+	race1 = l_timeout_create_ms(1100, race_handler, &race2, NULL);
+	race2 = l_timeout_create_ms(1100, race_handler, &race1, NULL);
 
 	remove_self = l_timeout_create(2, remove_handler, &remove_self, NULL);
 
-- 
2.10.2

[PATCH 5/5] unit: Add millisecond timeout overflow check

[Mat Martineau]

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

---
 unit/test-main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/unit/test-main.c b/unit/test-main.c
index bd68599..48cb16a 100644
--- a/unit/test-main.c
+++ b/unit/test-main.c
@@ -26,6 +26,8 @@
 
 #include <ell/ell.h>
 #include <unistd.h>
+#include <assert.h>
+#include <limits.h>
 
 static void signal_handler(struct l_signal *signal, uint32_t signo,
 							void *user_data)
@@ -116,6 +118,12 @@ int main(int argc, char *argv[])
 
 	l_debug("hello");
 
+#if (ULONG_MAX > UINT_MAX)
+	l_debug("Checking timeout time limit");
+	assert(!l_timeout_create_ms((UINT_MAX + 1UL) * 1000,
+					timeout_quit_handler, NULL, NULL));
+#endif
+
 	l_idle_oneshot(oneshot_handler, NULL, NULL);
 
 	l_main_run();
-- 
2.10.2

Re: [PATCH 1/5] string: Add l_string_unwrap and simplify l_string_free

[Denis Kenzior]

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

Hi Mat,

 > +/**
> + * l_string_unwrap:
> + * @string: growable string object
>    * @free_data: internal string array

I also squashed this line.  Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] unit: Update for l_string_free changes

[Denis Kenzior]

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

Hi Mat,

On 11/02/2016 04:50 PM, Mat Martineau wrote:
> ---
>   unit/test-dbus-service.c | 10 +++++-----
>   unit/test-string.c       | 10 +++++-----
>   2 files changed, 10 insertions(+), 10 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 3/5] timeout: Add millisecond timeout functions, remove nanosecond functions

[Denis Kenzior]

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

Hi Mat,

On 11/02/2016 04:50 PM, Mat Martineau wrote:
> Nanosecond resolution was overkill, but there are still common use cases
> for millisecond timer resolution.
> ---
>   ell/timeout.c | 109 +++++++++++++++++++++++++++++++++++++++++-----------------
>   ell/timeout.h |   8 ++---
>   2 files changed, 82 insertions(+), 35 deletions(-)
>

Patches 3-5 applied, thanks.

Regards,
-Denis