Re: [OpenWrt-Devel] [PATCH libubox/uloop] Remove signal handle after uloop_run()
On 2013-10-21 7:21 PM, Kristian Evensen wrote: > Hi, > > On Mon, Oct 21, 2013 at 5:31 PM, Felix Fietkau wrote: >> bool is a more appropriate type here than uint8_t > > Thanks for letting me know. > >> I think you need to use a different approach. What you're doing here >> breaks recursive uloop_run calls, which are used in a few places. >> You need to change the code to save/restore signal handlers instead of >> just setting them to NULL. > > Thanks for your comments. The more I think about this, the more I > believe that this is better dealt with by the user applications. One > simple approach would be to store the old sigaction (when entering > uloop first time) somewhere, and only guarantee that signal handling > is restored to what it was when uloop_run() was called the first time. > However, this would not cover all cases. Since you allow recursive > calls, I guess we have to store the signal handler + flags for each > call, to make sure no information is lost. Would approach one be > sufficient, or is number two required? I think storing the signal handler once and skipping the apply/restore for recursive calls should work. You could use a simple static counter to track the recursion level. - Felix ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH libubox/uloop] Remove signal handle after uloop_run()
Hi, On Mon, Oct 21, 2013 at 5:31 PM, Felix Fietkau wrote: > bool is a more appropriate type here than uint8_t Thanks for letting me know. > I think you need to use a different approach. What you're doing here > breaks recursive uloop_run calls, which are used in a few places. > You need to change the code to save/restore signal handlers instead of > just setting them to NULL. Thanks for your comments. The more I think about this, the more I believe that this is better dealt with by the user applications. One simple approach would be to store the old sigaction (when entering uloop first time) somewhere, and only guarantee that signal handling is restored to what it was when uloop_run() was called the first time. However, this would not cover all cases. Since you allow recursive calls, I guess we have to store the signal handler + flags for each call, to make sure no information is lost. Would approach one be sufficient, or is number two required? -Kristian ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH libubox/uloop] Remove signal handle after uloop_run()
On 2013-10-21 4:43 PM, Kristian Evensen wrote:
> From: Kristian Evensen
>
> uloop_run calls uloop_setup_signals() to set up signal handling before the
> while
> loop, but does not remove the signal handling after the loop has ended. This
> can
> cause problems for for example applications using the ubus file descriptor in
> their own event loops, and perhaps with their own signal handling.
>
> One use-case I experienced was an application that subscribed to several ubus
> objects and used the ubus file descriptor in its own event loop. Even though
> ubus_register_subscriber() (which calls uloop_run()) had returned, the signal
> handler was not removed. This caused SIGINT not to be caught by the
> application.
>
> I am not sure if adding the add variable to uloop_run() is required, or if it
> is
> sufficient to send 0/1 directly.
>
> Signed-off-by: Kristian Evensen
>
> ---
> uloop.c | 17 +
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/uloop.c b/uloop.c
> index 9a0590f..a3e95f7 100644
> --- a/uloop.c
> +++ b/uloop.c
> @@ -556,17 +556,24 @@ static void uloop_sigchld(int signo)
> do_sigchld = true;
> }
>
> -static void uloop_setup_signals(void)
> +static void uloop_setup_signals(uint8_t add)
bool is a more appropriate type here than uint8_t
> {
> struct sigaction s;
>
> memset(&s, 0, sizeof(struct sigaction));
> - s.sa_handler = uloop_handle_sigint;
> + if(add)
> + s.sa_handler = uloop_handle_sigint;
> + else
> + s.sa_handler = NULL;
> s.sa_flags = 0;
> sigaction(SIGINT, &s, NULL);
>
> if (uloop_handle_sigchld) {
> - s.sa_handler = uloop_sigchld;
> + if(add)
> + s.sa_handler = uloop_sigchld;
> + else
> + s.sa_handler = NULL;
> +
> sigaction(SIGCHLD, &s, NULL);
> }
> }
I think you need to use a different approach. What you're doing here
breaks recursive uloop_run calls, which are used in a few places.
You need to change the code to save/restore signal handlers instead of
just setting them to NULL.
- Felix
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
[OpenWrt-Devel] [PATCH libubox/uloop] Remove signal handle after uloop_run()
From: Kristian Evensen
uloop_run calls uloop_setup_signals() to set up signal handling before the while
loop, but does not remove the signal handling after the loop has ended. This can
cause problems for for example applications using the ubus file descriptor in
their own event loops, and perhaps with their own signal handling.
One use-case I experienced was an application that subscribed to several ubus
objects and used the ubus file descriptor in its own event loop. Even though
ubus_register_subscriber() (which calls uloop_run()) had returned, the signal
handler was not removed. This caused SIGINT not to be caught by the application.
I am not sure if adding the add variable to uloop_run() is required, or if it is
sufficient to send 0/1 directly.
Signed-off-by: Kristian Evensen
---
uloop.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/uloop.c b/uloop.c
index 9a0590f..a3e95f7 100644
--- a/uloop.c
+++ b/uloop.c
@@ -556,17 +556,24 @@ static void uloop_sigchld(int signo)
do_sigchld = true;
}
-static void uloop_setup_signals(void)
+static void uloop_setup_signals(uint8_t add)
{
struct sigaction s;
memset(&s, 0, sizeof(struct sigaction));
- s.sa_handler = uloop_handle_sigint;
+ if(add)
+ s.sa_handler = uloop_handle_sigint;
+ else
+ s.sa_handler = NULL;
s.sa_flags = 0;
sigaction(SIGINT, &s, NULL);
if (uloop_handle_sigchld) {
- s.sa_handler = uloop_sigchld;
+ if(add)
+ s.sa_handler = uloop_sigchld;
+ else
+ s.sa_handler = NULL;
+
sigaction(SIGCHLD, &s, NULL);
}
}
@@ -622,8 +629,9 @@ static void uloop_clear_processes(void)
void uloop_run(void)
{
struct timeval tv;
+ uint8_t add = 1;
- uloop_setup_signals();
+ uloop_setup_signals(add);
while(!uloop_cancelled)
{
uloop_gettime(&tv);
@@ -636,6 +644,7 @@ void uloop_run(void)
uloop_gettime(&tv);
uloop_run_events(uloop_get_next_timeout(&tv));
}
+ uloop_setup_signals(!add);
}
void uloop_done(void)
--
1.8.3.2
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
