[PATCH 1/3] main: Manually initialize and clean up the main loop

[PATCH 1/3] main: Manually initialize and clean up the main loop

[Mat Martineau]

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

Automatic initialization and cleanup of the main loop created some
issues with orderly shutdown of an ELL program. If cleanup functions
were called after the l_main_run() returned, they could indirectly set
up the epoll file descriptor and associated resources again. Those
resources were not freed after the cleanup functions ran.

Now, epoll resources are set up by l_main_init() and cleaned up by
l_main_exit(). If cleanup functions set up any idles or watches
(including watch-based timeouts, signals, or ios) after l_main_run()
returns, those will be destroyed by l_main_exit().

l_main_init() must be called before l_main_run() or any other ELL
function that expects epoll to be set up. l_main_exit() should be
called after l_main_run() returns to ensure that all resources are
freed.
---
 ell/main.c                   | 81 +++++++++++++++++++++++++++-----------------
 ell/main.h                   |  4 ++-
 examples/dbus-client.c       |  5 +++
 examples/dbus-service.c      |  5 +++
 examples/https-client-test.c |  5 +++
 examples/https-server-test.c |  5 +++
 unit/test-dbus-message-fds.c |  5 +++
 unit/test-dbus-properties.c  |  5 +++
 unit/test-dbus.c             |  5 +++
 unit/test-genl.c             |  5 +++
 unit/test-io.c               |  5 +++
 unit/test-kdbus.c            |  5 +++
 unit/test-main.c             |  5 +++
 unit/test-netlink.c          |  5 +++
 14 files changed, 113 insertions(+), 32 deletions(-)

diff --git a/ell/main.c b/ell/main.c
index f53ed9e..b4c1582 100644
--- a/ell/main.c
+++ b/ell/main.c
@@ -86,9 +86,6 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
 {
 	unsigned int i;
 
-	if (likely(epoll_fd))
-		return true;
-
 	epoll_fd = epoll_create1(EPOLL_CLOEXEC);
 	if (epoll_fd < 0) {
 		epoll_fd = 0;
@@ -127,7 +124,7 @@ int watch_add(int fd, uint32_t events, watch_event_cb_t callback,
 	if (unlikely(fd < 0 || !callback))
 		return -EINVAL;
 
-	if (!create_epoll())
+	if (!epoll_fd)
 		return -EIO;
 
 	if ((unsigned int) fd > watch_entries - 1)
@@ -260,7 +257,7 @@ int idle_add(idle_event_cb_t callback, void *user_data,
 	if (unlikely(!callback))
 		return -EINVAL;
 
-	if (!create_epoll())
+	if (!epoll_fd)
 		return -EIO;
 
 	data = l_new(struct idle_data, 1);
@@ -313,25 +310,45 @@ static void idle_dispatch(void *data, void *user_data)
 }
 
 /**
+ * l_main_init:
+ *
+ * Initialize the main loop. This must be called before l_main_run()
+ * and any other function that directly or indirectly sets up an idle
+ * or watch. A safe rule-of-thumb is to call it before any function
+ * prefixed with "l_".
+ *
+ * Returns: true if initialization was successful, false otherwise.
+ **/
+LIB_EXPORT bool l_main_init(void)
+{
+	if (unlikely(epoll_running))
+		return false;
+
+	if (!create_epoll())
+		return false;
+
+	epoll_terminate = false;
+
+	return true;
+}
+
+/**
  * l_main_run:
  *
  * Run the main loop
  *
- * Returns: #EXIT_SCUCESS after successful execution or #EXIT_FAILURE in
+ * Returns: #EXIT_SUCCESS after successful execution or #EXIT_FAILURE in
  *          case of failure
  **/
 LIB_EXPORT int l_main_run(void)
 {
-	unsigned int i;
-
-	if (unlikely(epoll_running))
+	/* Has l_main_init() been called? */
+	if (unlikely(!epoll_fd))
 		return EXIT_FAILURE;
 
-	if (!create_epoll())
+	if (unlikely(epoll_running))
 		return EXIT_FAILURE;
 
-	epoll_terminate = false;
-
 	epoll_running = true;
 
 	for (;;) {
@@ -375,6 +392,26 @@ LIB_EXPORT int l_main_run(void)
 		l_queue_foreach_remove(idle_list, idle_prune, NULL);
 	}
 
+	epoll_running = false;
+
+	return exit_status;
+}
+
+/**
+ * l_main_exit:
+ *
+ * Clean up after main loop completes.
+ *
+ **/
+LIB_EXPORT bool l_main_exit(void)
+{
+	unsigned int i;
+
+	if (epoll_running) {
+		l_error("Cleanup attempted on running main loop");
+		return false;
+	}
+
 	for (i = 0; i < watch_entries; i++) {
 		struct watch_data *data = watch_list[i];
 
@@ -399,12 +436,10 @@ LIB_EXPORT int l_main_run(void)
 	l_queue_destroy(idle_list, idle_destroy);
 	idle_list = NULL;
 
-	epoll_running = false;
-
 	close(epoll_fd);
 	epoll_fd = 0;
 
-	return exit_status;
+	return true;
 }
 
 /**
@@ -472,19 +507,3 @@ int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data)
 
 	return result;
 }
-
-/**
- * l_main_exit:
- *
- * Teminate the running main loop with exit status
- *
- **/
-LIB_EXPORT
-void l_main_exit(int status)
-{
-	if (unlikely(!epoll_running))
-		return;
-
-	exit_status = status;
-	epoll_terminate = true;
-}
diff --git a/ell/main.h b/ell/main.h
index 46d2a13..c23ec55 100644
--- a/ell/main.h
+++ b/ell/main.h
@@ -29,13 +29,15 @@
 extern "C" {
 #endif
 
+bool l_main_init(void);
 int l_main_run(void);
+bool l_main_exit(void);
+
 bool l_main_quit(void);
 
 typedef void (*l_main_signal_cb_t) (uint32_t signo, void *user_data);
 
 int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data);
-void l_main_exit(int status);
 
 #ifdef __cplusplus
 }
diff --git a/examples/dbus-client.c b/examples/dbus-client.c
index be640b7..507a4e8 100644
--- a/examples/dbus-client.c
+++ b/examples/dbus-client.c
@@ -78,6 +78,9 @@ int main(int argc, char *argv[])
 	sigset_t mask;
 	uint32_t watch_id;
 
+	if (!l_main_init())
+		return -1;
+
 	sigemptyset(&mask);
 	sigaddset(&mask, SIGINT);
 	sigaddset(&mask, SIGTERM);
@@ -102,5 +105,7 @@ int main(int argc, char *argv[])
 	l_dbus_destroy(dbus);
 	l_signal_remove(signal);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/examples/dbus-service.c b/examples/dbus-service.c
index 51b7e4f..19b17a0 100644
--- a/examples/dbus-service.c
+++ b/examples/dbus-service.c
@@ -181,6 +181,9 @@ int main(int argc, char *argv[])
 	sigset_t mask;
 	struct test_data *test;
 
+	if (!l_main_init())
+		return -1;
+
 	sigemptyset(&mask);
 	sigaddset(&mask, SIGINT);
 	sigaddset(&mask, SIGTERM);
@@ -233,5 +236,7 @@ cleanup:
 	l_dbus_destroy(dbus);
 	l_signal_remove(signal);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/examples/https-client-test.c b/examples/https-client-test.c
index 3382970..04a3b37 100644
--- a/examples/https-client-test.c
+++ b/examples/https-client-test.c
@@ -159,6 +159,9 @@ int main(int argc, char *argv[])
 		return -1;
 	}
 
+	if (!l_main_init())
+		return -1;
+
 	io = l_io_new(fd);
 	l_io_set_close_on_destroy(io, true);
 	l_io_set_read_handler(io, https_io_read, tls, NULL);
@@ -176,5 +179,7 @@ int main(int argc, char *argv[])
 	l_io_destroy(io);
 	l_tls_free(tls);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/examples/https-server-test.c b/examples/https-server-test.c
index 22a9db1..0f16ed2 100644
--- a/examples/https-server-test.c
+++ b/examples/https-server-test.c
@@ -149,6 +149,9 @@ int main(int argc, char *argv[])
 		return -1;
 	}
 
+	if (!l_main_init())
+		return -1;
+
 	io = l_io_new(fd);
 	l_io_set_close_on_destroy(io, true);
 	l_io_set_read_handler(io, https_io_read, tls, NULL);
@@ -164,5 +167,7 @@ int main(int argc, char *argv[])
 	l_io_destroy(io);
 	l_tls_free(tls);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/unit/test-dbus-message-fds.c b/unit/test-dbus-message-fds.c
index 7e710cf..c93f093 100644
--- a/unit/test-dbus-message-fds.c
+++ b/unit/test-dbus-message-fds.c
@@ -330,6 +330,9 @@ int main(int argc, char *argv[])
 	sigset_t mask;
 	int i;
 
+	if (!l_main_init())
+		return -1;
+
 	test_add("FD passing 1", test_fd_passing_1, NULL);
 
 	sigemptyset(&mask);
@@ -377,6 +380,8 @@ done:
 
 	l_queue_destroy(tests, l_free);
 
+	l_main_exit();
+
 	if (!success)
 		abort();
 
diff --git a/unit/test-dbus-properties.c b/unit/test-dbus-properties.c
index 6eae592..b11d1d5 100644
--- a/unit/test-dbus-properties.c
+++ b/unit/test-dbus-properties.c
@@ -978,6 +978,9 @@ int main(int argc, char *argv[])
 	sigset_t mask;
 	int i;
 
+	if (!l_main_init())
+		return -1;
+
 	test_add("Legacy properties get", test_old_get, NULL);
 	test_add("Legacy properties set", test_old_set, NULL);
 	test_add("Legacy optional property", test_old_optional_get, NULL);
@@ -1034,6 +1037,8 @@ done:
 
 	l_queue_destroy(tests, l_free);
 
+	l_main_exit();
+
 	if (!success)
 		abort();
 
diff --git a/unit/test-dbus.c b/unit/test-dbus.c
index b01a530..0fed605 100644
--- a/unit/test-dbus.c
+++ b/unit/test-dbus.c
@@ -201,6 +201,9 @@ int main(int argc, char *argv[])
 	sigset_t mask;
 	int i;
 
+	if (!l_main_init())
+		return -1;
+
 	sigemptyset(&mask);
 	sigaddset(&mask, SIGINT);
 	sigaddset(&mask, SIGTERM);
@@ -248,5 +251,7 @@ int main(int argc, char *argv[])
 
 	l_signal_remove(signal);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/unit/test-genl.c b/unit/test-genl.c
index 1764133..d15d92c 100644
--- a/unit/test-genl.c
+++ b/unit/test-genl.c
@@ -44,6 +44,9 @@ int main(int argc, char *argv[])
 {
 	struct l_genl *genl;
 
+	if (!l_main_init())
+		return -1;
+
 	l_log_set_stderr();
 
 	genl = l_genl_new_default();
@@ -56,5 +59,7 @@ int main(int argc, char *argv[])
 
 	l_genl_unref(genl);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/unit/test-io.c b/unit/test-io.c
index 24e1681..922ecad 100644
--- a/unit/test-io.c
+++ b/unit/test-io.c
@@ -75,6 +75,9 @@ int main(int argc, char *argv[])
 	struct l_io *io1, *io2;
 	int fd[2];
 
+	if (!l_main_init())
+		return -1;
+
 	l_log_set_stderr();
 
 	if (socketpair(PF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, fd) < 0) {
@@ -99,5 +102,7 @@ int main(int argc, char *argv[])
 	l_io_destroy(io2);
 	l_io_destroy(io1);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/unit/test-kdbus.c b/unit/test-kdbus.c
index 8cac761..ad4a754 100644
--- a/unit/test-kdbus.c
+++ b/unit/test-kdbus.c
@@ -136,6 +136,9 @@ int main(int argc, char *argv[])
 	struct l_signal *signal;
 	sigset_t mask;
 
+	if (!l_main_init())
+		return -1;
+
 	sigemptyset(&mask);
 	sigaddset(&mask, SIGINT);
 	sigaddset(&mask, SIGTERM);
@@ -191,5 +194,7 @@ error:
 
 	l_signal_remove(signal);
 
+	l_main_exit();
+
 	return EXIT_SUCCESS;
 }
diff --git a/unit/test-main.c b/unit/test-main.c
index ee99a4b..1f4bb31 100644
--- a/unit/test-main.c
+++ b/unit/test-main.c
@@ -91,6 +91,9 @@ int main(int argc, char *argv[])
 	struct l_idle *idle;
 	sigset_t mask;
 
+	if (!l_main_init())
+		return -1;
+
 	sigemptyset(&mask);
 	sigaddset(&mask, SIGINT);
 	sigaddset(&mask, SIGTERM);
@@ -129,5 +132,7 @@ int main(int argc, char *argv[])
 
 	l_idle_remove(idle);
 
+	l_main_exit();
+
 	return 0;
 }
diff --git a/unit/test-netlink.c b/unit/test-netlink.c
index a56e5a1..ce522a6 100644
--- a/unit/test-netlink.c
+++ b/unit/test-netlink.c
@@ -77,6 +77,9 @@ int main(int argc, char *argv[])
 	struct l_netlink *netlink;
 	struct ifinfomsg msg;
 
+	if (!l_main_init())
+		return -1;
+
 	l_log_set_stderr();
 
 	netlink = l_netlink_new(NETLINK_ROUTE);
@@ -92,5 +95,7 @@ int main(int argc, char *argv[])
 
 	l_netlink_destroy(netlink);
 
+	l_main_exit();
+
 	return 0;
 }
-- 
2.8.4

[PATCH 2/3] signal: Propagate watch_add errors in l_signal_create

[Mat Martineau]

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

l_signal_create now fails cleanly and returns NULL if watch_add fails.
---
 ell/signal.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/ell/signal.c b/ell/signal.c
index 863129f..fe4da55 100644
--- a/ell/signal.c
+++ b/ell/signal.c
@@ -155,6 +155,7 @@ LIB_EXPORT struct l_signal *l_signal_create(const sigset_t *mask,
 			void *user_data, l_signal_destroy_cb_t destroy)
 {
 	struct l_signal *signal;
+	int err;
 
 	if (unlikely(!mask || !callback))
 		return NULL;
@@ -172,16 +173,21 @@ LIB_EXPORT struct l_signal *l_signal_create(const sigset_t *mask,
 	}
 
 	signal->fd = signalfd(-1, mask, SFD_NONBLOCK | SFD_CLOEXEC);
-	if (signal->fd < 0) {
-		masked_signals_del(mask);
-		l_free(signal);
-		return NULL;
-	}
+	if (signal->fd < 0)
+		goto error;
 
-	watch_add(signal->fd, EPOLLIN, signal_callback,
-					signal, signal_destroy);
+	err = watch_add(signal->fd, EPOLLIN, signal_callback,
+			signal, signal_destroy);
+
+	if (err < 0)
+		goto error;
 
 	return signal;
+
+error:
+	masked_signals_del(mask);
+	l_free(signal);
+	return NULL;
 }
 
 /**
-- 
2.8.4

[PATCH 3/3] timeout: Propagate watch_add errors in l_timeout_create functions

[Mat Martineau]

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

l_timeout_create functions now free resources and return NULL if
watch_add fails.
---
 ell/timeout.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ell/timeout.c b/ell/timeout.c
index 4ba4e6e..fc24d60 100644
--- a/ell/timeout.c
+++ b/ell/timeout.c
@@ -135,6 +135,7 @@ LIB_EXPORT struct l_timeout *l_timeout_create_with_nanoseconds(unsigned int seco
 			void *user_data, l_timeout_destroy_cb_t destroy)
 {
 	struct l_timeout *timeout;
+	int err;
 
 	if (unlikely(!callback))
 		return NULL;
@@ -160,8 +161,13 @@ LIB_EXPORT struct l_timeout *l_timeout_create_with_nanoseconds(unsigned int seco
 		}
 	}
 
-	watch_add(timeout->fd, EPOLLIN | EPOLLONESHOT, timeout_callback,
-						timeout, timeout_destroy);
+	err = watch_add(timeout->fd, EPOLLIN | EPOLLONESHOT, timeout_callback,
+			timeout, timeout_destroy);
+
+	if (err < 0) {
+		l_free(timeout);
+		return NULL;
+	}
 
 	return timeout;
 }
-- 
2.8.4

Re: [PATCH 1/3] main: Manually initialize and clean up the main loop

[Denis Kenzior]

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

Hi Mat,

On 06/07/2016 05:30 PM, Mat Martineau wrote:
> Automatic initialization and cleanup of the main loop created some
> issues with orderly shutdown of an ELL program. If cleanup functions
> were called after the l_main_run() returned, they could indirectly set
> up the epoll file descriptor and associated resources again. Those
> resources were not freed after the cleanup functions ran.
>
> Now, epoll resources are set up by l_main_init() and cleaned up by
> l_main_exit(). If cleanup functions set up any idles or watches
> (including watch-based timeouts, signals, or ios) after l_main_run()
> returns, those will be destroyed by l_main_exit().
>
> l_main_init() must be called before l_main_run() or any other ELL
> function that expects epoll to be set up. l_main_exit() should be
> called after l_main_run() returns to ensure that all resources are
> freed.
> ---
>   ell/main.c                   | 81 +++++++++++++++++++++++++++-----------------
>   ell/main.h                   |  4 ++-
>   examples/dbus-client.c       |  5 +++
>   examples/dbus-service.c      |  5 +++
>   examples/https-client-test.c |  5 +++
>   examples/https-server-test.c |  5 +++
>   unit/test-dbus-message-fds.c |  5 +++
>   unit/test-dbus-properties.c  |  5 +++
>   unit/test-dbus.c             |  5 +++
>   unit/test-genl.c             |  5 +++
>   unit/test-io.c               |  5 +++
>   unit/test-kdbus.c            |  5 +++
>   unit/test-main.c             |  5 +++
>   unit/test-netlink.c          |  5 +++

Can you do me a favor and split these into three patches, according to 
HACKING 'Submitting Patches'.

>   14 files changed, 113 insertions(+), 32 deletions(-)
>
> diff --git a/ell/main.c b/ell/main.c
> index f53ed9e..b4c1582 100644
> --- a/ell/main.c
> +++ b/ell/main.c
> @@ -86,9 +86,6 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
>   {
>   	unsigned int i;
>
> -	if (likely(epoll_fd))
> -		return true;
> -
>   	epoll_fd = epoll_create1(EPOLL_CLOEXEC);
>   	if (epoll_fd < 0) {
>   		epoll_fd = 0;
> @@ -127,7 +124,7 @@ int watch_add(int fd, uint32_t events, watch_event_cb_t callback,
>   	if (unlikely(fd < 0 || !callback))
>   		return -EINVAL;
>
> -	if (!create_epoll())
> +	if (!epoll_fd)
>   		return -EIO;
>
>   	if ((unsigned int) fd > watch_entries - 1)
> @@ -260,7 +257,7 @@ int idle_add(idle_event_cb_t callback, void *user_data,
>   	if (unlikely(!callback))
>   		return -EINVAL;
>
> -	if (!create_epoll())
> +	if (!epoll_fd)
>   		return -EIO;
>
>   	data = l_new(struct idle_data, 1);
> @@ -313,25 +310,45 @@ static void idle_dispatch(void *data, void *user_data)
>   }
>
>   /**
> + * l_main_init:
> + *
> + * Initialize the main loop. This must be called before l_main_run()
> + * and any other function that directly or indirectly sets up an idle
> + * or watch. A safe rule-of-thumb is to call it before any function
> + * prefixed with "l_".
> + *
> + * Returns: true if initialization was successful, false otherwise.
> + **/
> +LIB_EXPORT bool l_main_init(void)
> +{
> +	if (unlikely(epoll_running))
> +		return false;
> +
> +	if (!create_epoll())
> +		return false;
> +
> +	epoll_terminate = false;
> +
> +	return true;
> +}
> +
> +/**
>    * l_main_run:
>    *
>    * Run the main loop

Should we mention that the main loop can be restarted?

e.g.
l_main_run
l_main_quit
l_main_run

is possible?

>    *
> - * Returns: #EXIT_SCUCESS after successful execution or #EXIT_FAILURE in
> + * Returns: #EXIT_SUCCESS after successful execution or #EXIT_FAILURE in
>    *          case of failure
>    **/
>   LIB_EXPORT int l_main_run(void)
>   {
> -	unsigned int i;
> -
> -	if (unlikely(epoll_running))
> +	/* Has l_main_init() been called? */
> +	if (unlikely(!epoll_fd))
>   		return EXIT_FAILURE;
>
> -	if (!create_epoll())
> +	if (unlikely(epoll_running))
>   		return EXIT_FAILURE;
>
> -	epoll_terminate = false;
> -
>   	epoll_running = true;
>
>   	for (;;) {
> @@ -375,6 +392,26 @@ LIB_EXPORT int l_main_run(void)
>   		l_queue_foreach_remove(idle_list, idle_prune, NULL);
>   	}
>
> +	epoll_running = false;
> +
> +	return exit_status;

Do we still need this global exit_status variable?  Or should we always 
just return 0 here?

> +}
> +
> +/**
> + * l_main_exit:
> + *
> + * Clean up after main loop completes.
> + *
> + **/
> +LIB_EXPORT bool l_main_exit(void)
> +{
> +	unsigned int i;
> +
> +	if (epoll_running) {
> +		l_error("Cleanup attempted on running main loop");
> +		return false;
> +	}
> +
>   	for (i = 0; i < watch_entries; i++) {
>   		struct watch_data *data = watch_list[i];
>
> @@ -399,12 +436,10 @@ LIB_EXPORT int l_main_run(void)
>   	l_queue_destroy(idle_list, idle_destroy);
>   	idle_list = NULL;
>
> -	epoll_running = false;
> -
>   	close(epoll_fd);
>   	epoll_fd = 0;
>
> -	return exit_status;
> +	return true;
>   }
>
>   /**
> @@ -472,19 +507,3 @@ int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data)
>
>   	return result;
>   }
> -
> -/**
> - * l_main_exit:
> - *
> - * Teminate the running main loop with exit status
> - *
> - **/
> -LIB_EXPORT
> -void l_main_exit(int status)
> -{
> -	if (unlikely(!epoll_running))
> -		return;
> -
> -	exit_status = status;
> -	epoll_terminate = true;
> -}
> diff --git a/ell/main.h b/ell/main.h
> index 46d2a13..c23ec55 100644
> --- a/ell/main.h
> +++ b/ell/main.h
> @@ -29,13 +29,15 @@
>   extern "C" {
>   #endif
>
> +bool l_main_init(void);
>   int l_main_run(void);
> +bool l_main_exit(void);
> +

I wonder if returning bool here is useful since you never check the 
return value in any of the actual files that use this.

>   bool l_main_quit(void);
>
>   typedef void (*l_main_signal_cb_t) (uint32_t signo, void *user_data);
>
>   int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data);
> -void l_main_exit(int status);
>
>   #ifdef __cplusplus
>   }

Regards,
-Denis

Re: [PATCH 2/3] signal: Propagate watch_add errors in l_signal_create

[Denis Kenzior]

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

Hi Mat,

On 06/07/2016 05:30 PM, Mat Martineau wrote:
> l_signal_create now fails cleanly and returns NULL if watch_add fails.
> ---
>   ell/signal.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
>

Applied after tweaking the commit header.

Regards,
-Denis

Re: [PATCH 3/3] timeout: Propagate watch_add errors in l_timeout_create functions

[Denis Kenzior]

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

Hi Mat,

On 06/07/2016 05:30 PM, Mat Martineau wrote:
> l_timeout_create functions now free resources and return NULL if
> watch_add fails.
> ---
>   ell/timeout.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 1/3] main: Manually initialize and clean up the main loop

[Mat Martineau]

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

On Wed, 8 Jun 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 06/07/2016 05:30 PM, Mat Martineau wrote:
>> Automatic initialization and cleanup of the main loop created some
>> issues with orderly shutdown of an ELL program. If cleanup functions
>> were called after the l_main_run() returned, they could indirectly set
>> up the epoll file descriptor and associated resources again. Those
>> resources were not freed after the cleanup functions ran.
>> 
>> Now, epoll resources are set up by l_main_init() and cleaned up by
>> l_main_exit(). If cleanup functions set up any idles or watches
>> (including watch-based timeouts, signals, or ios) after l_main_run()
>> returns, those will be destroyed by l_main_exit().
>> 
>> l_main_init() must be called before l_main_run() or any other ELL
>> function that expects epoll to be set up. l_main_exit() should be
>> called after l_main_run() returns to ensure that all resources are
>> freed.
>> ---
>>   ell/main.c                   | 81 
>> +++++++++++++++++++++++++++-----------------
>>   ell/main.h                   |  4 ++-
>>   examples/dbus-client.c       |  5 +++
>>   examples/dbus-service.c      |  5 +++
>>   examples/https-client-test.c |  5 +++
>>   examples/https-server-test.c |  5 +++
>>   unit/test-dbus-message-fds.c |  5 +++
>>   unit/test-dbus-properties.c  |  5 +++
>>   unit/test-dbus.c             |  5 +++
>>   unit/test-genl.c             |  5 +++
>>   unit/test-io.c               |  5 +++
>>   unit/test-kdbus.c            |  5 +++
>>   unit/test-main.c             |  5 +++
>>   unit/test-netlink.c          |  5 +++
>
> Can you do me a favor and split these into three patches, according to 
> HACKING 'Submitting Patches'.

What do you know, "patches should be split even if breaking compilation is 
unavoidable". Curious about why this is (I'm not lobbying for a change, 
it's just not self-evident).

>
>>   14 files changed, 113 insertions(+), 32 deletions(-)
>> 
>> diff --git a/ell/main.c b/ell/main.c
>> index f53ed9e..b4c1582 100644
>> --- a/ell/main.c
>> +++ b/ell/main.c
>> @@ -86,9 +86,6 @@ static inline bool __attribute__ ((always_inline)) 
>> create_epoll(void)
>>   {
>>   	unsigned int i;
>> 
>> -	if (likely(epoll_fd))
>> -		return true;
>> -
>>   	epoll_fd = epoll_create1(EPOLL_CLOEXEC);
>>   	if (epoll_fd < 0) {
>>   		epoll_fd = 0;
>> @@ -127,7 +124,7 @@ int watch_add(int fd, uint32_t events, watch_event_cb_t 
>> callback,
>>   	if (unlikely(fd < 0 || !callback))
>>   		return -EINVAL;
>> 
>> -	if (!create_epoll())
>> +	if (!epoll_fd)
>>   		return -EIO;
>>
>>   	if ((unsigned int) fd > watch_entries - 1)
>> @@ -260,7 +257,7 @@ int idle_add(idle_event_cb_t callback, void *user_data,
>>   	if (unlikely(!callback))
>>   		return -EINVAL;
>> 
>> -	if (!create_epoll())
>> +	if (!epoll_fd)
>>   		return -EIO;
>>
>>   	data = l_new(struct idle_data, 1);
>> @@ -313,25 +310,45 @@ static void idle_dispatch(void *data, void 
>> *user_data)
>>   }
>>
>>   /**
>> + * l_main_init:
>> + *
>> + * Initialize the main loop. This must be called before l_main_run()
>> + * and any other function that directly or indirectly sets up an idle
>> + * or watch. A safe rule-of-thumb is to call it before any function
>> + * prefixed with "l_".
>> + *
>> + * Returns: true if initialization was successful, false otherwise.
>> + **/
>> +LIB_EXPORT bool l_main_init(void)
>> +{
>> +	if (unlikely(epoll_running))
>> +		return false;
>> +
>> +	if (!create_epoll())
>> +		return false;
>> +
>> +	epoll_terminate = false;
>> +
>> +	return true;
>> +}
>> +
>> +/**
>>    * l_main_run:
>>    *
>>    * Run the main loop
>
> Should we mention that the main loop can be restarted?
>
> e.g.
> l_main_run
> l_main_quit
> l_main_run
>
> is possible?

Yeah, worth noting.

>
>>    *
>> - * Returns: #EXIT_SCUCESS after successful execution or #EXIT_FAILURE in
>> + * Returns: #EXIT_SUCCESS after successful execution or #EXIT_FAILURE in
>>    *          case of failure
>>    **/
>>   LIB_EXPORT int l_main_run(void)
>>   {
>> -	unsigned int i;
>> -
>> -	if (unlikely(epoll_running))
>> +	/* Has l_main_init() been called? */
>> +	if (unlikely(!epoll_fd))
>>   		return EXIT_FAILURE;
>> 
>> -	if (!create_epoll())
>> +	if (unlikely(epoll_running))
>>   		return EXIT_FAILURE;
>> 
>> -	epoll_terminate = false;
>> -
>>   	epoll_running = true;
>>
>>   	for (;;) {
>> @@ -375,6 +392,26 @@ LIB_EXPORT int l_main_run(void)
>>   		l_queue_foreach_remove(idle_list, idle_prune, NULL);
>>   	}
>> 
>> +	epoll_running = false;
>> +
>> +	return exit_status;
>
> Do we still need this global exit_status variable?  Or should we always just 
> return 0 here?

The only way to reach that return statement now involves calling 
l_main_quit, so I agree that it's not needed any more.

>
>> +}
>> +
>> +/**
>> + * l_main_exit:
>> + *
>> + * Clean up after main loop completes.
>> + *
>> + **/
>> +LIB_EXPORT bool l_main_exit(void)
>> +{
>> +	unsigned int i;
>> +
>> +	if (epoll_running) {
>> +		l_error("Cleanup attempted on running main loop");
>> +		return false;
>> +	}
>> +
>>   	for (i = 0; i < watch_entries; i++) {
>>   		struct watch_data *data = watch_list[i];
>> 
>> @@ -399,12 +436,10 @@ LIB_EXPORT int l_main_run(void)
>>   	l_queue_destroy(idle_list, idle_destroy);
>>   	idle_list = NULL;
>> 
>> -	epoll_running = false;
>> -
>>   	close(epoll_fd);
>>   	epoll_fd = 0;
>> 
>> -	return exit_status;
>> +	return true;
>>   }
>>
>>   /**
>> @@ -472,19 +507,3 @@ int l_main_run_with_signal(l_main_signal_cb_t 
>> callback, void *user_data)
>>
>>   	return result;
>>   }
>> -
>> -/**
>> - * l_main_exit:
>> - *
>> - * Teminate the running main loop with exit status
>> - *
>> - **/
>> -LIB_EXPORT
>> -void l_main_exit(int status)
>> -{
>> -	if (unlikely(!epoll_running))
>> -		return;
>> -
>> -	exit_status = status;
>> -	epoll_terminate = true;
>> -}
>> diff --git a/ell/main.h b/ell/main.h
>> index 46d2a13..c23ec55 100644
>> --- a/ell/main.h
>> +++ b/ell/main.h
>> @@ -29,13 +29,15 @@
>>   extern "C" {
>>   #endif
>> 
>> +bool l_main_init(void);
>>   int l_main_run(void);
>> +bool l_main_exit(void);
>> +
>
> I wonder if returning bool here is useful since you never check the return 
> value in any of the actual files that use this.

All of the unit tests and examples exit out of main immediately after 
calling l_main_exit, but that may not be the case for every program. 
l_main_exit will do nothing if the main loop is still running.

l_main_run and l_main_quit's return values are similarly ignored, but are 
potentially useful in a similar way.

>
>>   bool l_main_quit(void);
>>
>>   typedef void (*l_main_signal_cb_t) (uint32_t signo, void *user_data);
>>
>>   int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data);
>> -void l_main_exit(int status);
>>
>>   #ifdef __cplusplus
>>   }

--
Mat Martineau
Intel OTC

Re: [PATCH 1/3] main: Manually initialize and clean up the main loop

[Denis Kenzior]

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

Hi Mat,

 >>
>> Can you do me a favor and split these into three patches, according to
>> HACKING 'Submitting Patches'.
>
> What do you know, "patches should be split even if breaking compilation
> is unavoidable". Curious about why this is (I'm not lobbying for a
> change, it's just not self-evident).
>

We hardly ever use git bisect, so readability of commits trumps the 
breaking of compilation.

>>
>> I wonder if returning bool here is useful since you never check the
>> return value in any of the actual files that use this.
>
> All of the unit tests and examples exit out of main immediately after
> calling l_main_exit, but that may not be the case for every program.
> l_main_exit will do nothing if the main loop is still running.
>
> l_main_run and l_main_quit's return values are similarly ignored, but
> are potentially useful in a similar way.
>

Fair enough, just curious.  Some like to use void *_exit functions 
because you can't do anything if stuff fails anyway.  I've had this 
argument with myself many times and don't really have a preference.

Regards,
-Denis