Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-25 Thread Ed Schouten
2012/10/25 Konstantin Belousov :
> Release is performed on the separate branch, which has currently nothing
> to do with stable/9. The laster is open for normal MFC procedures, so I
> do not see how the merge of bugfix to stable is related to the release.

I wasn't sure whether there still was some kind of freeze in effect or
not. Thanks for clarifying.

-- 
Ed Schouten 
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-25 Thread Konstantin Belousov
On Thu, Oct 25, 2012 at 11:06:09AM +0200, Ed Schouten wrote:
> 2012/10/25 Jeremy Chadwick :
> > I assume a commit to HEAD + MFC in 2 weeks is in order?
> 
> Yes. We're far too late to get this into 9.1, so I'll MFC it after the 
> release.
> 
> Patch committed as r242078!

Release is performed on the separate branch, which has currently nothing
to do with stable/9. The laster is open for normal MFC procedures, so I
do not see how the merge of bugfix to stable is related to the release.


pgpjKGEVy4JAS.pgp
Description: PGP signature


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-25 Thread Ed Schouten
2012/10/25 Jeremy Chadwick :
> I assume a commit to HEAD + MFC in 2 weeks is in order?

Yes. We're far too late to get this into 9.1, so I'll MFC it after the release.

Patch committed as r242078!

-- 
Ed Schouten 
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-25 Thread Jeremy Chadwick
On Thu, Oct 25, 2012 at 09:45:11AM +0200, Ed Schouten wrote:
> 2012/10/23 Ed Schouten :
> > Will try to come up with a decent patch tomorrow evening.
> 
> Ahem; the day after tomorrow. Jeremy, could you please try the following 
> patch?
> 
> http://80386.nl/pub/tty-bg-read.txt
> 
> I decomposed the TTY read routine into four separate functions to
> improve clarity. While this was initially true, I think it's a pity
> the four functions are constantly becoming a bit more complex.
> 
> The same issue is also present on the output path, but I have no idea
> how realistic/hard it is to fix this issue. Also, it might not really
> be an issue in practice. If you do a large write and become a
> non-foreground process group, you might be able to circumvent TOSTOP
> while the write() is in transit.
> 
> Fixing this might be tedious, because we currently enforce that writes
> to a TTY are serialized. Blocking inside the write() might then cause
> a deadlock. But in my opinion, I would prefer the serialization over
> the enforcing of TOSTOP.

After the patch, testing with grep and cat, and checking cv/state for
the latter case:


(01:30:39 jdc@icarus) ~ $ csh
% grep -r "-2011" . &
[1] 1964
% jobs
[1]  + Suspended (tty input) grep -r -2011 .
% fg
grep -r -2011 .
^C
% jobs

% grep -r "-2011" .
^Z
Suspended
% bg
[1]grep -r -2011 . &
[1]  + Suspended (tty input) grep -r -2011 .
% fg
grep -r -2011 .
^C

% cat &
[1] 2042
% jobs
[1]  + Suspended (tty input) cat
% ps -auxwU jdc | grep cat
jdc  2042  0.0  0.0  9908 1496  1  T 1:34AM 0:00.00 cat
jdc  2044  0.0  0.0 16268 1864  1  S+1:34AM 0:00.00 grep cat
% top -b -U jdc | grep cat
 2042 jdc 1  200  9908K  1496K STOP0   0:00  0.00% cat
 2047 jdc 1  200 16268K  1864K piperd  0   0:00  0.00% grep cat
% fg
cat
^C
% exit


I do not get dropped characters or witness any other anomalies.  I
tested behaviour with /bin/sh, as well as bash.  All seems good.

> Thanks again for reporting the issue!

No, thank *you* and others for looking + fixing it!  :-)

I assume a commit to HEAD + MFC in 2 weeks is in order?

I'll update the PR with this part of our mail thread.

-- 
| Jeremy Chadwick   j...@koitsu.org |
| UNIX Systems Administratorhttp://jdc.koitsu.org/ |
| Mountain View, CA, US|
| Making life hard for others since 1977. PGP 4BD6C0CB |
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-25 Thread Ed Schouten
Hi all,

2012/10/23 Ed Schouten :
> Will try to come up with a decent patch tomorrow evening.

Ahem; the day after tomorrow. Jeremy, could you please try the following patch?

http://80386.nl/pub/tty-bg-read.txt

I decomposed the TTY read routine into four separate functions to
improve clarity. While this was initially true, I think it's a pity
the four functions are constantly becoming a bit more complex.

The same issue is also present on the output path, but I have no idea
how realistic/hard it is to fix this issue. Also, it might not really
be an issue in practice. If you do a large write and become a
non-foreground process group, you might be able to circumvent TOSTOP
while the write() is in transit.

Fixing this might be tedious, because we currently enforce that writes
to a TTY are serialized. Blocking inside the write() might then cause
a deadlock. But in my opinion, I would prefer the serialization over
the enforcing of TOSTOP.

Thanks again for reporting the issue!

-- 
Ed Schouten 
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-23 Thread Jeremy Chadwick
On Tue, Oct 23, 2012 at 08:13:55PM -0700, Jeremy Chadwick wrote:
> On Tue, Oct 23, 2012 at 11:07:56PM -0400, Eitan Adler wrote:
> > On 23 October 2012 22:40, Jeremy Chadwick  wrote:
> > 
> > > No problem folks.  Would you like me to file a PR for this so we can
> > > track it/for historical purposes?
> > 
> > yes please
> 
> Thanks Eitan -- will do!

kern/173010

-- 
| Jeremy Chadwick   j...@koitsu.org |
| UNIX Systems Administratorhttp://jdc.koitsu.org/ |
| Mountain View, CA, US|
| Making life hard for others since 1977. PGP 4BD6C0CB |
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-23 Thread Jeremy Chadwick
On Tue, Oct 23, 2012 at 11:07:56PM -0400, Eitan Adler wrote:
> On 23 October 2012 22:40, Jeremy Chadwick  wrote:
> 
> > No problem folks.  Would you like me to file a PR for this so we can
> > track it/for historical purposes?
> 
> yes please

Thanks Eitan -- will do!

-- 
| Jeremy Chadwick   j...@koitsu.org |
| UNIX Systems Administratorhttp://jdc.koitsu.org/ |
| Mountain View, CA, US|
| Making life hard for others since 1977. PGP 4BD6C0CB |
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-23 Thread Eitan Adler
On 23 October 2012 22:40, Jeremy Chadwick  wrote:

> No problem folks.  Would you like me to file a PR for this so we can
> track it/for historical purposes?

yes please



-- 
Eitan Adler
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-23 Thread Jeremy Chadwick
On Tue, Oct 23, 2012 at 10:42:47PM +0200, Ed Schouten wrote:
> Hi Kostik,
> 
> 2012/10/23 Konstantin Belousov :
> > This is reproducable with the cat(1) as well. The telling part is that
> > the backgrounded process stays on the "ttyin" cv. The code for e.g.
> > tty read currently is structured as follows:
> > check for background process reading from CTTY, send SIGTTYIN
> > loop {
> > sleep waiting for input
> > process input
> > }
> > The problem is that the SIGCONT does not remove the sleeping process from
> > the sleep queue, so the sleep is not interrupted with error. Instead, the
> > process is woken up later when input is available.
> >
> > Old tty code did the recheck for background state inside the loop after
> > the sleep.
> 
> Exactly. Was just debugging this as well and came to the same
> conclusion. Will try to come up with a decent patch tomorrow evening.
> 
> Thanks for reporting the issue!

No problem folks.  Would you like me to file a PR for this so we can
track it/for historical purposes?

-- 
| Jeremy Chadwick   j...@koitsu.org |
| UNIX Systems Administratorhttp://jdc.koitsu.org/ |
| Mountain View, CA, US|
| Making life hard for others since 1977. PGP 4BD6C0CB |
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-23 Thread Ed Schouten
Hi Kostik,

2012/10/23 Konstantin Belousov :
> This is reproducable with the cat(1) as well. The telling part is that
> the backgrounded process stays on the "ttyin" cv. The code for e.g.
> tty read currently is structured as follows:
> check for background process reading from CTTY, send SIGTTYIN
> loop {
> sleep waiting for input
> process input
> }
> The problem is that the SIGCONT does not remove the sleeping process from
> the sleep queue, so the sleep is not interrupted with error. Instead, the
> process is woken up later when input is available.
>
> Old tty code did the recheck for background state inside the loop after
> the sleep.

Exactly. Was just debugging this as well and came to the same
conclusion. Will try to come up with a decent patch tomorrow evening.

Thanks for reporting the issue!

-- 
Ed Schouten 
___
freebsd-stable@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"


Re: pty/tty or signal strangeness, or grep/bsdgrep bug?

2012-10-23 Thread Konstantin Belousov
On Tue, Oct 23, 2012 at 07:27:03AM -0700, Jeremy Chadwick wrote:
> Please keep me CC'd as I'm not subscribed to the list.
> 
> Something "fun" today.  First off: yes, I should have been using
> 'bsdgrep -r -- "-2011" .', and yes that works fine, but that's besides
> the point.  Here we go:
> 
> % bsdgrep -r "-2011" .
> ^Z
> Suspended
> % bg
> [1]bsdgrep -r -2011 . &
> % qqq
> %
> % fg(standard input):qfgfg
> 
> bsdgrep -r -2011 .
> ^C
> %
> 
> Let me explain what transpired from an input perspective:
> 
> 1. Ran bsdgrep -r "-2011" .
> 2. Pressed Ctrl-Z
> 3. Typed bg
> 4. Typed "q" 20 times in a row exactly
> 5. Pressed Ctrl-C
> 6. Typed "fg" and pressed Enter
> 7. Typed "ffgg" and pressed Enter
> 8. Pressed Ctrl-Z and Enter
> 9. Pressed Ctrl-C
> 
> What's going on here?  Where is the famous "Suspended (tty input)"?  It
> gets more interesting.  Here's another one:
> 
> % bsdgrep -r "-2011" .
> ^Z
> Suspended
> % bg
> [1]bsdgrep -r -2011 . &
> % g(standard input):f
> fg
> gfg: Command not found.
> [1]  + Suspended (tty input) bsdgrep -r -2011 .
> % jobs
> [1]  + Suspended (tty input) bsdgrep -r -2011 .
> % fg
> bsdgrep -r -2011 .
> %
> 
> And what transpired input-wise:
> 
> 1. Ran bsdgrep -r "-2011" .
> 2. Pressed Ctrl-Z
> 3. Typed bg
> 4. Typed "fg" and Enter
> 5. Typed "fg" again and pressed Enter
> 6. Typed "jobs" and pressed Enter
> 7. Typed "fg" and pressed Enter
> 8. Pressed Ctrl-D
> 
> Some facts:
> 
> - Fully 100% reproducible
> - Tested only on RELENG_9 (source from 2012/10/21)
> - Happens regardless of shell (bash and csh tested; csh w/out dot files)
> - Similar behaviour happens with our base system grep (GNU grep) but
>   sometimes it manifests itself in a weirder way
> - bsdgrep and GNU grep are both in state "ttyin" when this happens
> 
> CC'ing some folks who might have some ideas or can explain how to
> troubleshoot this one.  For the signal part, I believe this would be
> SIGTTIN.

This is reproducable with the cat(1) as well. The telling part is that
the backgrounded process stays on the "ttyin" cv. The code for e.g.
tty read currently is structured as follows:
check for background process reading from CTTY, send SIGTTYIN
loop {
sleep waiting for input
process input
}
The problem is that the SIGCONT does not remove the sleeping process from
the sleep queue, so the sleep is not interrupted with error. Instead, the
process is woken up later when input is available.

Old tty code did the recheck for background state inside the loop after
the sleep.

Below is the hacky change that seemingly helped for exactly your case.
New code is structured so that the fix requires big movements of blocks,
e.g. even if keeping my patch, the for (;;) loops in tty_ttydisc.c
no longer have any use.

Hope Ed will comment.

diff --git a/sys/kern/tty.c b/sys/kern/tty.c
index e9c0fb6..3785e81 100644
--- a/sys/kern/tty.c
+++ b/sys/kern/tty.c
@@ -434,6 +434,7 @@ ttydev_read(struct cdev *dev, struct uio *uio, int ioflag)
if (error)
goto done;
 
+check_bg:
error = tty_wait_background(tp, curthread, SIGTTIN);
if (error) {
tty_unlock(tp);
@@ -441,6 +442,8 @@ ttydev_read(struct cdev *dev, struct uio *uio, int ioflag)
}
 
error = ttydisc_read(tp, uio, ioflag);
+   if (error == EJUSTRETURN)
+   goto check_bg;
tty_unlock(tp);
 
/*
@@ -462,6 +465,7 @@ ttydev_write(struct cdev *dev, struct uio *uio, int ioflag)
if (error)
return (error);
 
+check_bg:
if (tp->t_termios.c_lflag & TOSTOP) {
error = tty_wait_background(tp, curthread, SIGTTOU);
if (error)
@@ -484,6 +488,8 @@ ttydev_write(struct cdev *dev, struct uio *uio, int ioflag)
tp->t_flags &= ~TF_BUSY_OUT;
cv_signal(&tp->t_outserwait);
}
+   if (error == EJUSTRETURN)
+   goto check_bg;
 
 done:  tty_unlock(tp);
return (error);
diff --git a/sys/kern/tty_ttydisc.c b/sys/kern/tty_ttydisc.c
index 52a5df2..d8b8f53 100644
--- a/sys/kern/tty_ttydisc.c
+++ b/sys/kern/tty_ttydisc.c
@@ -157,7 +157,7 @@ ttydisc_read_canonical(struct tty *tp, struct uio *uio, int 
ioflag)
error = tty_wait(tp, &tp->t_inwait);
if (error)
return (error);
-   continue;
+   return (EJUSTRETURN);
}
 
/* Don't send the EOF char back to userspace. */
@@ -208,6 +208,7 @@ ttydisc_read_raw_no_timer(struct tty *tp, struct uio *uio, 
int ioflag)
error = tty_wait(tp, &tp->t_inwait);
if (error)
return (error);
+   return (EJUSTRETURN);
}
 }
 
@@ -256,6 +257,7 @@ ttydisc_read_raw_read_timer(struct tty *tp, struct uio 
*uio, int ioflag,
error = tty_timedwait(