diff mbox

[OpenWrt-Devel,libubox,2/3] loop: make uloop_run() return the cancelling signal

Message ID 22d591176c7b3512ceabddf439a6d6ab682288d1.1466409587.git.mschiffer@universe-factory.net
State Superseded
Headers show

Commit Message

Matthias Schiffer June 20, 2016, 7:59 a.m. UTC
When a process quits in response to a signal it handles, it should to so
be re-sending the signal to itself. This especially important for SIGINT,
as is explained in [1].

uloop currently hides the reason for quitting uloop_run(). Fix this by
returning the signal that caused the loop to quit (or 0 when uloop_end()
was used), so a program using loop an comply with [1].

uloop_cancelled is renamed to uloop_status, so accidentially running a
process compiled with one uloop_cancelled definition against the other is
not possible.

[1] https://www.cons.org/cracauer/sigint.html

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 uloop.c | 14 ++++++++------
 uloop.h |  6 +++---
 2 files changed, 11 insertions(+), 9 deletions(-)

Comments

Matthias Schiffer June 20, 2016, 9:11 a.m. UTC | #1
On 06/20/2016 09:59 AM, Matthias Schiffer wrote:
> When a process quits in response to a signal it handles, it should to so
> be re-sending the signal to itself. This especially important for SIGINT,
> as is explained in [1].
> 
> uloop currently hides the reason for quitting uloop_run(). Fix this by
> returning the signal that caused the loop to quit (or 0 when uloop_end()
> was used), so a program using loop an comply with [1].
> 
> uloop_cancelled is renamed to uloop_status, so accidentially running a
> process compiled with one uloop_cancelled definition against the other is
> not possible.

I've just noticed that this will also require a ubus change, as libubus is
accessing uloop_cancelled directly.

Would you prefer to keep uloop_cancelled with its old definition and add
ubus_status as an additional variable to keep this compatible? An
alternative would be to provide a helper uloop_is_cancelled() based on
ubus_status and adjust libubus accordingly.

Regards,
Matthias

> 
> [1] https://www.cons.org/cracauer/sigint.html
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
Jo-Philipp Wich June 21, 2016, 10:34 a.m. UTC | #2
On 06/20/2016 09:59 AM, Matthias Schiffer wrote:
> When a process quits in response to a signal it handles, it should to so
> be re-sending the signal to itself. This especially important for SIGINT,
> as is explained in [1].
> 
> uloop currently hides the reason for quitting uloop_run(). Fix this by
> returning the signal that caused the loop to quit (or 0 when uloop_end()
> was used), so a program using loop an comply with [1].
> 
> uloop_cancelled is renamed to uloop_status, so accidentially running a
> process compiled with one uloop_cancelled definition against the other is
> not possible.
> 
> [1] https://www.cons.org/cracauer/sigint.html
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
Acked-by: Jo-Philipp Wich <jo@mein.io>
> ---
>  uloop.c | 14 ++++++++------
>  uloop.h |  6 +++---
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/uloop.c b/uloop.c
> index cd3de85..fa3544d 100644
> --- a/uloop.c
> +++ b/uloop.c
> @@ -57,7 +57,7 @@ static struct list_head timeouts = LIST_HEAD_INIT(timeouts);
>  static struct list_head processes = LIST_HEAD_INIT(processes);
>  
>  static int poll_fd = -1;
> -bool uloop_cancelled = false;
> +int uloop_status = -1;
>  static bool do_sigchld = false;
>  
>  static struct uloop_fd_event cur_fds[ULOOP_MAX_EVENTS];
> @@ -330,7 +330,7 @@ static void uloop_handle_processes(void)
>  
>  static void uloop_handle_sigint(int signo)
>  {
> -	uloop_cancelled = true;
> +	uloop_status = signo;
>  }
>  
>  static void uloop_sigchld(int signo)
> @@ -443,7 +443,7 @@ static void uloop_clear_processes(void)
>  		uloop_process_delete(p);
>  }
>  
> -void uloop_run(void)
> +int uloop_run(void)
>  {
>  	static int recursive_calls = 0;
>  	struct timeval tv;
> @@ -455,8 +455,8 @@ void uloop_run(void)
>  	if (!recursive_calls++)
>  		uloop_setup_signals(true);
>  
> -	uloop_cancelled = false;
> -	while(!uloop_cancelled)
> +	uloop_status = -1;
> +	while (uloop_status < 0)
>  	{
>  		uloop_gettime(&tv);
>  		uloop_process_timeouts(&tv);
> @@ -464,7 +464,7 @@ void uloop_run(void)
>  		if (do_sigchld)
>  			uloop_handle_processes();
>  
> -		if (uloop_cancelled)
> +		if (uloop_status >= 0)
>  			break;
>  
>  		uloop_gettime(&tv);
> @@ -473,6 +473,8 @@ void uloop_run(void)
>  
>  	if (!--recursive_calls)
>  		uloop_setup_signals(false);
> +
> +	return uloop_status;
>  }
>  
>  void uloop_done(void)
> diff --git a/uloop.h b/uloop.h
> index 7564514..4d87d46 100644
> --- a/uloop.h
> +++ b/uloop.h
> @@ -83,7 +83,7 @@ struct uloop_process
>  	pid_t pid;
>  };
>  
> -extern bool uloop_cancelled;
> +extern int uloop_status;
>  extern bool uloop_handle_sigchld;
>  
>  int uloop_fd_add(struct uloop_fd *sock, unsigned int flags);
> @@ -99,11 +99,11 @@ int uloop_process_delete(struct uloop_process *p);
>  
>  static inline void uloop_end(void)
>  {
> -	uloop_cancelled = true;
> +	uloop_status = 0;
>  }
>  
>  int uloop_init(void);
> -void uloop_run(void);
> +int uloop_run(void);
>  void uloop_done(void);
>  
>  #endif
>
diff mbox

Patch

diff --git a/uloop.c b/uloop.c
index cd3de85..fa3544d 100644
--- a/uloop.c
+++ b/uloop.c
@@ -57,7 +57,7 @@  static struct list_head timeouts = LIST_HEAD_INIT(timeouts);
 static struct list_head processes = LIST_HEAD_INIT(processes);
 
 static int poll_fd = -1;
-bool uloop_cancelled = false;
+int uloop_status = -1;
 static bool do_sigchld = false;
 
 static struct uloop_fd_event cur_fds[ULOOP_MAX_EVENTS];
@@ -330,7 +330,7 @@  static void uloop_handle_processes(void)
 
 static void uloop_handle_sigint(int signo)
 {
-	uloop_cancelled = true;
+	uloop_status = signo;
 }
 
 static void uloop_sigchld(int signo)
@@ -443,7 +443,7 @@  static void uloop_clear_processes(void)
 		uloop_process_delete(p);
 }
 
-void uloop_run(void)
+int uloop_run(void)
 {
 	static int recursive_calls = 0;
 	struct timeval tv;
@@ -455,8 +455,8 @@  void uloop_run(void)
 	if (!recursive_calls++)
 		uloop_setup_signals(true);
 
-	uloop_cancelled = false;
-	while(!uloop_cancelled)
+	uloop_status = -1;
+	while (uloop_status < 0)
 	{
 		uloop_gettime(&tv);
 		uloop_process_timeouts(&tv);
@@ -464,7 +464,7 @@  void uloop_run(void)
 		if (do_sigchld)
 			uloop_handle_processes();
 
-		if (uloop_cancelled)
+		if (uloop_status >= 0)
 			break;
 
 		uloop_gettime(&tv);
@@ -473,6 +473,8 @@  void uloop_run(void)
 
 	if (!--recursive_calls)
 		uloop_setup_signals(false);
+
+	return uloop_status;
 }
 
 void uloop_done(void)
diff --git a/uloop.h b/uloop.h
index 7564514..4d87d46 100644
--- a/uloop.h
+++ b/uloop.h
@@ -83,7 +83,7 @@  struct uloop_process
 	pid_t pid;
 };
 
-extern bool uloop_cancelled;
+extern int uloop_status;
 extern bool uloop_handle_sigchld;
 
 int uloop_fd_add(struct uloop_fd *sock, unsigned int flags);
@@ -99,11 +99,11 @@  int uloop_process_delete(struct uloop_process *p);
 
 static inline void uloop_end(void)
 {
-	uloop_cancelled = true;
+	uloop_status = 0;
 }
 
 int uloop_init(void);
-void uloop_run(void);
+int uloop_run(void);
 void uloop_done(void);
 
 #endif