On Mon, Jan 5, 2015 at 11:01 AM, Ted Unangst <[email protected]> wrote:
...
> In the regular timeout case, I'm not sure what you're changing. There
> is a problem here though. If we're already close to the timeout
> expiring, we shouldn't sleep the full timeout, only the time left
> since we began the read.
>
> Looking at the current code, the comparison seems reversed. If we've
> just started reading, ticks == rdStart, and so adding the timeout will
> always be greater, not less, than the current ticks.
>
> I believe this diff addresses all the above issues. The current code
> works correctly for the rtout == 0 case, but sleeping for only the
> remaining timeout is much easier done by splitting it.
>
> Anyone else have thoughts? I get confused by this code every time I
> read it, perhaps because bugs obscure what it's trying to do and what
> is doing. Am I correct?
>
> Index: bpf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/bpf.c,v
> retrieving revision 1.113
> diff -u -p -r1.113 bpf.c
> --- bpf.c 16 Dec 2014 18:30:04 -0000 1.113
> +++ bpf.c 5 Jan 2015 18:49:11 -0000
> @@ -430,10 +430,13 @@ bpfread(dev_t dev, struct uio *uio, int
> if (d->bd_rtout == -1) {
> /* User requested non-blocking I/O */
> error = EWOULDBLOCK;
> + } else if (d->bd_rtout == 0) {
> + error = tsleep(d, PRINET|PCATCH, "bpf", 0);
> } else {
> - if ((d->bd_rdStart + d->bd_rtout) < ticks) {
> - error = tsleep((caddr_t)d, PRINET|PCATCH,
> "bpf",
> - d->bd_rtout);
> + int elapsed = ticks - d->bd_rdStart;
> + if (elapsed < d->bd_rtout) {
> + error = tsleep(d, PRINET|PCATCH, "bpf",
> + d->bd_rtout - elapsed);
> } else
> error = EWOULDBLOCK;
> }
>
