From: <bla...@ya...> - 2005-05-16 20:28:37
|
Replace pause with sigsuspend, to avoid needing to set an empty handler for SIGWINCH. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- linux-2.6.git-paolo/arch/um/drivers/chan_user.c | 15 ++++----------- 1 files changed, 4 insertions(+), 11 deletions(-) diff -puN arch/um/drivers/chan_user.c~uml-replace-pause-with-sigsuspend arch/um/drivers/chan_user.c --- linux-2.6.git/arch/um/drivers/chan_user.c~uml-replace-pause-with-sigsuspend 2005-05-16 08:07:30.000000000 +0200 +++ linux-2.6.git-paolo/arch/um/drivers/chan_user.c 2005-05-16 08:08:10.000000000 +0200 @@ -67,10 +67,6 @@ error: * it (see below for how we make sure to exit only on SIGWINCH). */ -static void winch_handler(int sig) -{ -} - struct winch_data { int pty_fd; int pipe_fd; @@ -93,14 +89,11 @@ static int winch_thread(void *arg) printk("winch_thread : failed to write synchronization " "byte, err = %d\n", -count); - /* We are not using SIG_IGN on purpose, so don't fix it as I thought to - * do! If using SIG_IGN, the pause() call below would not stop on - * SIGWINCH. */ - - signal(SIGWINCH, winch_handler); sigfillset(&sigs); sigdelset(&sigs, SIGWINCH); - /* Block anything else than SIGWINCH. */ + /* Block anything else than SIGWINCH. XXX: Actually, this may be removed + * maybe, due to sigsuspend below, which replaces the signal mask, but + * let's keep it.*/ if(sigprocmask(SIG_SETMASK, &sigs, NULL) < 0){ printk("winch_thread : sigprocmask failed, errno = %d\n", errno); @@ -130,7 +123,7 @@ static int winch_thread(void *arg) while(1){ /* This will be interrupted by SIGWINCH only, since other signals * are blocked.*/ - pause(); + sigsuspend(&sigs); count = os_write_file(pipe_fd, &c, sizeof(c)); if(count != sizeof(c)) _ |
From: Bodo S. <bst...@fu...> - 2005-05-17 10:14:28
|
I didn't have the time to test the patch, but I guess, it won't work. Maybe, the man pages for sigsuspend are somewhat missleading, saying: The sigsuspend call temporarily replaces the signal mask for the process with that given by mask and then suspends the process until a signal is received. Reading the code of sys_sigsuspend() in arch/i386/kernel/signal.c, you'll find that it will return to user only, if do_signal() returns 1. This will happen, if there was a signal to deliver. Ignored signals will not be delivered, so sys_sigsupend will not return on SIGWINCH, if SIGWINCH handler is set to SIG_IGN. So, for UML's winch_handler, there is no difference between pause() and sigsuspend(), because sigsuspend's feature of accepting a sigmask for use while waiting, in fact isn't needed here as the same mask is set already before with sigprocmask(). Bodo bla...@ya... wrote: > Replace pause with sigsuspend, to avoid needing to set an empty handler for SIGWINCH. > > Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> > --- > > linux-2.6.git-paolo/arch/um/drivers/chan_user.c | 15 ++++----------- > 1 files changed, 4 insertions(+), 11 deletions(-) > > diff -puN arch/um/drivers/chan_user.c~uml-replace-pause-with-sigsuspend arch/um/drivers/chan_user.c > --- linux-2.6.git/arch/um/drivers/chan_user.c~uml-replace-pause-with-sigsuspend 2005-05-16 08:07:30.000000000 +0200 > +++ linux-2.6.git-paolo/arch/um/drivers/chan_user.c 2005-05-16 08:08:10.000000000 +0200 > @@ -67,10 +67,6 @@ error: > * it (see below for how we make sure to exit only on SIGWINCH). > */ > > -static void winch_handler(int sig) > -{ > -} > - > struct winch_data { > int pty_fd; > int pipe_fd; > @@ -93,14 +89,11 @@ static int winch_thread(void *arg) > printk("winch_thread : failed to write synchronization " > "byte, err = %d\n", -count); > > - /* We are not using SIG_IGN on purpose, so don't fix it as I thought to > - * do! If using SIG_IGN, the pause() call below would not stop on > - * SIGWINCH. */ > - > - signal(SIGWINCH, winch_handler); > sigfillset(&sigs); > sigdelset(&sigs, SIGWINCH); > - /* Block anything else than SIGWINCH. */ > + /* Block anything else than SIGWINCH. XXX: Actually, this may be removed > + * maybe, due to sigsuspend below, which replaces the signal mask, but > + * let's keep it.*/ > if(sigprocmask(SIG_SETMASK, &sigs, NULL) < 0){ > printk("winch_thread : sigprocmask failed, errno = %d\n", > errno); > @@ -130,7 +123,7 @@ static int winch_thread(void *arg) > while(1){ > /* This will be interrupted by SIGWINCH only, since other signals > * are blocked.*/ > - pause(); > + sigsuspend(&sigs); > > count = os_write_file(pipe_fd, &c, sizeof(c)); > if(count != sizeof(c)) > _ > > > ------------------------------------------------------- > This SF.Net email is sponsored by Oracle Space Sweepstakes > Want to be the first software developer in space? > Enter now for the Oracle Space Sweepstakes! > http://ads.osdn.com/?ad_id=7412&alloc_id=16344&op=click > _______________________________________________ > User-mode-linux-devel mailing list > Use...@li... > https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel |
From: Blaisorblade <bla...@ya...> - 2005-05-18 15:19:01
|
On Tuesday 17 May 2005 12:14, Bodo Stroesser wrote: > I didn't have the time to test the patch, but I guess, it won't work. > > Maybe, the man pages for sigsuspend are somewhat missleading, saying: > > The sigsuspend call temporarily replaces the signal mask for > the process with that given by mask and then suspends the > process until a signal is received. In fact, I assumed that was correct... > Reading the code of sys_sigsuspend() in arch/i386/kernel/signal.c, > you'll find that it will return to user only, if do_signal() returns 1. > This will happen, if there was a signal to deliver. Ignored signals > will not be delivered, so sys_sigsupend will not return on SIGWINCH, > if SIGWINCH handler is set to SIG_IGN. Doh! You're right, actually. > So, for UML's winch_handler, there is no difference between pause() > and sigsuspend(), because sigsuspend's feature of accepting a sigmask > for use while waiting, in fact isn't needed here as the same mask is > set already before with sigprocmask(). Ok, seems like I'll drop this patch. Thanks for the review. -- Paolo Giarrusso, aka Blaisorblade Skype user "PaoloGiarrusso" Linux registered user n. 292729 http://www.user-mode-linux.org/~blaisorblade |
From: Bodo S. <bst...@fu...> - 2005-05-18 19:19:38
Attachments:
fix-winch_thread.patch
|
Blaisorblade wrote: > On Tuesday 17 May 2005 12:14, Bodo Stroesser wrote: > >>I didn't have the time to test the patch, but I guess, it won't work. >> >>Maybe, the man pages for sigsuspend are somewhat missleading, saying: >> >> The sigsuspend call temporarily replaces the signal mask for >> the process with that given by mask and then suspends the >> process until a signal is received. > > In fact, I assumed that was correct... > >>Reading the code of sys_sigsuspend() in arch/i386/kernel/signal.c, >>you'll find that it will return to user only, if do_signal() returns 1. >>This will happen, if there was a signal to deliver. Ignored signals >>will not be delivered, so sys_sigsupend will not return on SIGWINCH, >>if SIGWINCH handler is set to SIG_IGN. > > Doh! You're right, actually. > >>So, for UML's winch_handler, there is no difference between pause() >>and sigsuspend(), because sigsuspend's feature of accepting a sigmask >>for use while waiting, in fact isn't needed here as the same mask is >>set already before with sigprocmask(). > > Ok, seems like I'll drop this patch. Thanks for the review. Maybe, we really should use sigsuspend() instead of pause(), but the reason for this isn't to remove the winch_handler. The current loop in winch_thread() might miss a SIGWINCH, if a SIGWINCH comes in while winch_thread() isn't waiting in wait(). So I think, winch_thread() should block all signals including SIGWINCH. In its loop it should call sigsuspend() with a mask as argument, that unblocks SIGWINCH while sigsuspend() waits. I've attached a patch (tested a bit only). Bodo |
From: Blaisorblade <bla...@ya...> - 2005-05-19 13:36:24
|
On Wednesday 18 May 2005 21:19, Bodo Stroesser wrote: > Blaisorblade wrote: > > On Tuesday 17 May 2005 12:14, Bodo Stroesser wrote: > >>I didn't have the time to test the patch, but I guess, it won't work. > >> > >>Maybe, the man pages for sigsuspend are somewhat missleading, saying: > >> > >> The sigsuspend call temporarily replaces the signal mask for > >> the process with that given by mask and then suspends the > >> process until a signal is received. > > > > In fact, I assumed that was correct... > > > >>Reading the code of sys_sigsuspend() in arch/i386/kernel/signal.c, > >>you'll find that it will return to user only, if do_signal() returns 1. > >>This will happen, if there was a signal to deliver. Ignored signals > >>will not be delivered, so sys_sigsupend will not return on SIGWINCH, > >>if SIGWINCH handler is set to SIG_IGN. > > > > Doh! You're right, actually. > > > >>So, for UML's winch_handler, there is no difference between pause() > >>and sigsuspend(), because sigsuspend's feature of accepting a sigmask > >>for use while waiting, in fact isn't needed here as the same mask is > >>set already before with sigprocmask(). > > > > Ok, seems like I'll drop this patch. Thanks for the review. > Maybe, we really should use sigsuspend() instead of pause(), but the > reason for this isn't to remove the winch_handler. > The current loop in winch_thread() might miss a SIGWINCH, if a SIGWINCH > comes in while winch_thread() isn't waiting in wait(). wait()? You mean the pause()/sigsuspend() call, right? Then I probably agree. > So I think, winch_thread() should block all signals including SIGWINCH. > In its loop it should call sigsuspend() with a mask as argument, that > unblocks SIGWINCH while sigsuspend() waits. > > I've attached a patch (tested a bit only). > > Bodo -- Paolo Giarrusso, aka Blaisorblade Skype user "PaoloGiarrusso" Linux registered user n. 292729 http://www.user-mode-linux.org/~blaisorblade |
From: Bodo S. <bst...@fu...> - 2005-05-19 13:39:49
|
Blaisorblade wrote: > On Wednesday 18 May 2005 21:19, Bodo Stroesser wrote: > >>Maybe, we really should use sigsuspend() instead of pause(), but the >>reason for this isn't to remove the winch_handler. >>The current loop in winch_thread() might miss a SIGWINCH, if a SIGWINCH >>comes in while winch_thread() isn't waiting in wait(). > > > wait()? You mean the pause()/sigsuspend() call, right? Then I probably agree. > Yes. You are right. Stupid typo. Bodo |