* [uml-devel] [patch 1/1] uml: replace pause with sigsuspend
@ 2005-05-16 18:01 blaisorblade
2005-05-17 10:14 ` Bodo Stroesser
0 siblings, 1 reply; 6+ messages in thread
From: blaisorblade @ 2005-05-16 18:01 UTC (permalink / raw)
To: jdike; +Cc: user-mode-linux-devel, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [uml-devel] [patch 1/1] uml: replace pause with sigsuspend
2005-05-16 18:01 [uml-devel] [patch 1/1] uml: replace pause with sigsuspend blaisorblade
@ 2005-05-17 10:14 ` Bodo Stroesser
2005-05-18 15:18 ` Blaisorblade
0 siblings, 1 reply; 6+ messages in thread
From: Bodo Stroesser @ 2005-05-17 10:14 UTC (permalink / raw)
To: blaisorblade; +Cc: jdike, user-mode-linux-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [uml-devel] [patch 1/1] uml: replace pause with sigsuspend
2005-05-17 10:14 ` Bodo Stroesser
@ 2005-05-18 15:18 ` Blaisorblade
2005-05-18 19:19 ` Bodo Stroesser
0 siblings, 1 reply; 6+ messages in thread
From: Blaisorblade @ 2005-05-18 15:18 UTC (permalink / raw)
To: user-mode-linux-devel; +Cc: Bodo Stroesser, jdike
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [uml-devel] [patch 1/1] uml: replace pause with sigsuspend
2005-05-18 15:18 ` Blaisorblade
@ 2005-05-18 19:19 ` Bodo Stroesser
2005-05-19 13:35 ` Blaisorblade
0 siblings, 1 reply; 6+ messages in thread
From: Bodo Stroesser @ 2005-05-18 19:19 UTC (permalink / raw)
To: Blaisorblade; +Cc: user-mode-linux-devel, jdike
[-- 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))
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [uml-devel] [patch 1/1] uml: replace pause with sigsuspend
2005-05-18 19:19 ` Bodo Stroesser
@ 2005-05-19 13:35 ` Blaisorblade
2005-05-19 13:39 ` Bodo Stroesser
0 siblings, 1 reply; 6+ messages in thread
From: Blaisorblade @ 2005-05-19 13:35 UTC (permalink / raw)
To: user-mode-linux-devel; +Cc: Bodo Stroesser, jdike
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [uml-devel] [patch 1/1] uml: replace pause with sigsuspend
2005-05-19 13:35 ` Blaisorblade
@ 2005-05-19 13:39 ` Bodo Stroesser
0 siblings, 0 replies; 6+ messages in thread
From: Bodo Stroesser @ 2005-05-19 13:39 UTC (permalink / raw)
To: Blaisorblade; +Cc: user-mode-linux-devel, jdike
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2005-05-19 13:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-05-16 18:01 [uml-devel] [patch 1/1] uml: replace pause with sigsuspend blaisorblade
2005-05-17 10:14 ` Bodo Stroesser
2005-05-18 15:18 ` Blaisorblade
2005-05-18 19:19 ` Bodo Stroesser
2005-05-19 13:35 ` Blaisorblade
2005-05-19 13:39 ` Bodo Stroesser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox