Bug 30985
| Summary: | incorrect signal handling in snpread() | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Valentin Nechayev <netch> |
| Component: | kern | Assignee: | freebsd-bugs (Nobody) <bugs> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | ||
| Priority: | Normal | ||
| Version: | 4.4-RELEASE | ||
| Hardware: | Any | ||
| OS: | Any | ||
|
Description
Valentin Nechayev
2001-10-02 14:00:01 UTC
>>Synopsis: incorrect signal handling in snpread()
VN> --- tty_snoop.c.0 Thu Nov 18 08:39:47 1999
VN> +++ tty_snoop.c Tue Oct 2 15:23:54 2001
For 5-current, the same bug is in src/sys/dev/snp/snp.c.
/netch
Valentin Nechayev <netch@lucky.net> wrote: > >Fix: > > Following simple fix provides checking for tsleep() return code > and EINTR returning when signal is caught. It is applicable for > all late 4.* systems. > It also increases IPL around the data wait cycle. As data can be put > to snoop device during interrupt (am I wrong?), it is desirable. I don't think it's possible for data to enter snp during an interrupt. At least, there are no provisions for this anywhere else in the code. Thus, I think this is undesirable. > But it should be noted that there are some principal architectural > issues in this approach. As snp device is designed for use when > select/poll is used to wait and ioctl(FIONREAD) is desired to get > tty status, it may be desirable to avoid blocking reads of snp device > totally. I can't suppose what approach is more right. I think it's okay to sleep in snpread. snp generally tries to act like any other driver, and most allow sleeping in their read routine. > do { > if (snp->snp_len == 0) { > - if (flag & IO_NDELAY) > + if (flag & IO_NDELAY) { > + splx(s); > return (EWOULDBLOCK); > + } > snp->snp_flags |= SNOOP_RWAIT; > - tsleep((caddr_t) snp, (PZERO + 1) | PCATCH, "snoopread" > , 0); > + error = tsleep((caddr_t) snp, (PZERO + 1) | PCATCH, "snoopread", 0); > + if (error == EINTR || error == ERESTART) { > + splx(s); > + return EINTR; > + } Why can't we just return whatever tsleep() returns, as most (all?) other drivers do? Like so (untested): Index: snp.c =================================================================== RCS file: /ref/cvsf/src/sys/dev/snp/snp.c,v retrieving revision 1.63 diff -u -r1.63 snp.c --- snp.c 2001/09/12 08:37:11 1.63 +++ snp.c 2001/10/07 15:44:47 @@ -255,7 +255,10 @@ if (flag & IO_NDELAY) return (EWOULDBLOCK); snp->snp_flags |= SNOOP_RWAIT; - tsleep((caddr_t)snp, (PZERO + 1) | PCATCH, "snprd", 0); + error = tsleep((caddr_t)snp, (PZERO + 1) | PCATCH, + "snprd", 0); + if (error != 0) + return (error); } } while (snp->snp_len == 0); Sun, Oct 07, 2001 at 08:50:41, dima wrote about "Re: kern/30985: incorrect signal handling in snpread()": > > - tsleep((caddr_t) snp, (PZERO + 1) | PCATCH, "snoopread" > > , 0); > > + error = tsleep((caddr_t) snp, (PZERO + 1) | PCATCH, "snoopread", 0); > > + if (error == EINTR || error == ERESTART) { > > + splx(s); > > + return EINTR; > > + } > > Why can't we just return whatever tsleep() returns, as most (all?) > other drivers do? Like so (untested): I am not experienced kernel hacker ;) The examples I saw in kernel code, mostly test for ERESTART and possibly EINTR and exit from routine in case of such codes. tsleep(9) man page (in RELENG_4_4) mentions the only another return code allowed - EWOULDBLOCK in timeout case, but snpread() doesn't suppose timeout. If you suppose that exit on any nonzero value is correct, you probably are right. > snp->snp_flags |= SNOOP_RWAIT; > - tsleep((caddr_t)snp, (PZERO + 1) | PCATCH, "snprd", 0); > + error = tsleep((caddr_t)snp, (PZERO + 1) | PCATCH, > + "snprd", 0); > + if (error != 0) > + return (error); > } /netch State Changed From-To: open->closed FIxed in -current, thanks! (And sorry for the delay.) |
