Date: Sun, 7 Oct 2001 09:00:02 -0700 (PDT) From: Dima Dorfman <dima@trit.org> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/30985: incorrect signal handling in snpread() Message-ID: <200110071600.f97G02O77773@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/30985; it has been noted by GNATS.
From: Dima Dorfman <dima@trit.org>
To: Valentin Nechayev <netch@segfault.kiev.ua>
Cc: FreeBSD-gnats-submit@freebsd.org
Subject: Re: kern/30985: incorrect signal handling in snpread()
Date: Sun, 07 Oct 2001 08:50:41 -0700
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);
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200110071600.f97G02O77773>