Re: [uml-devel] [patch 1/1] uml: replace pause with sigsuspend

[Bodo Stroesser <bstroesser@fujitsu-siemens.com>]

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

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




[-- Attachment #2: fix-winch_thread.patch --]
[-- Type: text/x-diff, Size: 1554 bytes --]


From: Bodo Stroesser <bstroesser@fujitsu-siemens.com>

If a SIGWINCH comes in, while winch_thread() isn't waiting
in wait(), winch_thread could miss signals.
It isn't very probable, that anyone will see this causing
trouble, as it would need a very special timing, that a
missed SIGWINCH results in a wrong window size.

So, this is a minor problem. But why not fix, as it can
be done so easy?

Signed-off-by: Bodo Stroesser <bstroesser@fujitsu-siemens.com>
---


diff -puN arch/um/drivers/chan_user.c~fix-winch_thread arch/um/drivers/chan_user.c
--- linux-2.6.12-rc4/arch/um/drivers/chan_user.c~fix-winch_thread	2005-05-18 20:53:32.000000000 +0200
+++ linux-2.6.12-rc4-root/arch/um/drivers/chan_user.c	2005-05-18 21:16:55.000000000 +0200
@@ -98,13 +98,14 @@ static int winch_thread(void *arg)
 
 	signal(SIGWINCH, winch_handler);
 	sigfillset(&sigs);
-	sigdelset(&sigs, SIGWINCH);
-	/* Block anything else than SIGWINCH. */
+	/* Block all signals possible. */
 	if(sigprocmask(SIG_SETMASK, &sigs, NULL) < 0){
 		printk("winch_thread : sigprocmask failed, errno = %d\n", 
 		       errno);
 		exit(1);
 	}
+	/* In sigsuspend(), block anything else than SIGWINCH. */
+	sigdelset(&sigs, SIGWINCH);
 
 	if(setsid() < 0){
 		printk("winch_thread : setsid failed, errno = %d\n", errno);
@@ -129,7 +130,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))
_