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

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

[blaisorblade]

Replace pause with sigsuspend, to avoid needing to set an empty handler for SIGWINCH.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 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
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

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

[Bodo Stroesser]

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

blaisorblade@yahoo.it wrote:
> Replace pause with sigsuspend, to avoid needing to set an empty handler for SIGWINCH.
> 
> Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> ---
> 
>  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
> User-mode-linux-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel


-------------------------------------------------------
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
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

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

[Blaisorblade]

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




-------------------------------------------------------
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
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

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

[Bodo Stroesser]

[-- 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))
_

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

[Blaisorblade]

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




-------------------------------------------------------
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
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

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

[Bodo Stroesser]

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


-------------------------------------------------------
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
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel