Skip site navigation (1)Skip section navigation (2) 
Date:      Wed, 11 Jan 2012 01:14:44 +0100
From:      Giovanni Trematerra <gianni@freebsd.org>
To:        Bruce Evans <brde@optusnet.com.au>
Cc:        jilles@freebsd.org, Attilio Rao <attilio@freebsd.org>, flo@freebsd.org, Konstantin Belousov <kib@freebsd.org>, freebsd-arch@freebsd.org
Subject:   Re: pipe/fifo code merged.
Message-ID:  <CACfq0922=y20Bo2ekp82DTv2BTqD%2BbJEYppK0nkbkef-9CuKdA@mail.gmail.com>
In-Reply-To: <20120110211510.T1676@besplex.bde.org>
References:  <CACfq093o9iVZKxCj58OR2hpCLDYTUTdxg_re_bEMYn2SrNrLCQ@mail.gmail.com> <20120110005155.S2378@besplex.bde.org> <CACfq09225iMYLe6p8jSiVhsDw_rqTyEHsvPdtZXLrQYT0-skzg@mail.gmail.com> <20120110153807.H943@besplex.bde.org> <20120110211510.T1676@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help 
On Tue, Jan 10, 2012 at 1:04 PM, Bruce Evans <brde@optusnet.com.au> wrote:
> On Tue, 10 Jan 2012, Bruce Evans wrote:
>
>> I think you don't want me to read the patch, since I would see too much
>> detail starting with style bugs. =A0Anyway..
>> ...
>
>
> One more set of details.
>
> % + =A0 =A0 =A0 =A0 =A0 =A0 PIPE_UNLOCK(rpipe);
> % + =A0 =A0 =A0 =A0 =A0 =A0 if (fifo_iseof(fp))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 events |=3D POLLINIGNEOF;
> % + =A0 =A0 =A0 =A0 =A0 =A0 PIPE_LOCK(rpipe);
>
> This is new code (needed to force POLLIGNEOF for fifos).
>
> It is a layering violation to call the fifo code for non-fifos here.
> fifo_iseof() handles this internally by checking fp->vnode->v_fifoinfo.
> The pipe layer should know if it is dealing with a fifo in a better way
> than that.

I fixed this in
http://www.trematerra.net/patches/pipefifo_merge2.4.diff

>
> I don't like unlocking in the middle in general, and here it gives
> races. =A0We will miss setting POLLIN | POLLRDNORM for certain changes
> if they weren't set earlier and the state changed while unlocked. =A0Why
> unlock anyway or lock in fifo_iseof()? =A0Only fi_seqcount =3D=3D fi_wgen
> is checked under the lock there. =A0Races in that check are probably
> just as harmless as races here. =A0And locking doesn't even prevent them,
> since if fi_seqcount or fi_wgen can change underneath us, they can also
> change just after we check them. =A0They rarely change compared with the
> buffer count raced with above.


fixed that too as you suggest.

> % @@ -1326,58 +1549,66 @@ pipe_poll(fp, events, active_cred, td)
> % =A0 =A0 =A0 struct ucred *active_cred;
> % =A0 =A0 =A0 struct thread *td;
> % =A0{
> % - =A0 =A0 struct pipe *rpipe =3D fp->f_data;
> % + =A0 =A0 struct pipeinfo *pip =3D fp->f_data;
> % + =A0 =A0 struct pipe *rpipe;
> % =A0 =A0 =A0 struct pipe *wpipe;
> % =A0 =A0 =A0 int revents =3D 0;
> % =A0#ifdef MAC
> % =A0 =A0 =A0 int error;
> % =A0#endif
> % % - =A0 wpipe =3D rpipe->pipe_peer;
>
> % + =A0 =A0 rpipe =3D pip->pi_rpipe;
> % + =A0 =A0 wpipe =3D pip->pi_wpipe->pipe_peer;
> % =A0 =A0 =A0 PIPE_LOCK(rpipe);
> % =A0#ifdef MAC
> % =A0 =A0 =A0 error =3D mac_pipe_check_poll(active_cred, rpipe->pipe_pair=
);
> % =A0 =A0 =A0 if (error)
> % - =A0 =A0 =A0 =A0 =A0 =A0 goto locked_error;
> % + =A0 =A0 =A0 =A0 =A0 =A0 return (0);
>
> Seems to be broken. =A0The unlock is now missing.

fixed.


>
> % -static int
> % -fifo_poll_f(struct file *fp, int events, struct ucred *cred, struct
> thread *td)
> % -{
> % - =A0 =A0 struct fifoinfo *fip;
> % - =A0 =A0 struct file filetmp;
> % - =A0 =A0 int levents, revents =3D 0;
> % -
> % - =A0 =A0 fip =3D fp->f_data;
> % - =A0 =A0 levents =3D events &
> % - =A0 =A0 =A0 =A0 (POLLIN | POLLINIGNEOF | POLLPRI | POLLRDNORM | POLLR=
DBAND);
> % - =A0 =A0 if ((fp->f_flag & FREAD) && levents) {
> % - =A0 =A0 =A0 =A0 =A0 =A0 filetmp.f_data =3D fip->fi_readsock;
> % - =A0 =A0 =A0 =A0 =A0 =A0 filetmp.f_cred =3D cred;
> % - =A0 =A0 =A0 =A0 =A0 =A0 mtx_lock(&fifo_mtx);
> % - =A0 =A0 =A0 =A0 =A0 =A0 if (fp->f_seqcount =3D=3D fip->fi_wgen)
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 levents |=3D POLLINIGNEOF;
> % - =A0 =A0 =A0 =A0 =A0 =A0 mtx_unlock(&fifo_mtx);
> % - =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D soo_poll(&filetmp, levents, cred=
, td);
> % - =A0 =A0 }
> % - =A0 =A0 levents =3D events & (POLLOUT | POLLWRNORM | POLLWRBAND);
> % - =A0 =A0 if ((fp->f_flag & FWRITE) && levents) {
> % - =A0 =A0 =A0 =A0 =A0 =A0 filetmp.f_data =3D fip->fi_writesock;
> % - =A0 =A0 =A0 =A0 =A0 =A0 filetmp.f_cred =3D cred;
> % - =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D soo_poll(&filetmp, levents, cred=
, td);
> % - =A0 =A0 }
> % - =A0 =A0 return (revents);
> % -}
>
> This was reasonably clean. =A0My version is cleaner:
> - POLLIGNEOF is an old mistake of mine. =A0I tried to kill it, but kib@
> =A0propagated it to sys_pipe.c too, where it has survived another release
> =A0or two. =A0In my version, I still have it in the call to soo_poll() bu=
t
> =A0don't have it in the `levents =3D events & ...' mask. =A0Thus it is a
> =A0pure kernel flag, and acts the same as your isfifo flag -- it tells
> =A0the socket layer to do something unusual because this is a fifo. =A0It
> =A0is not needed any more, since the pipe layer is close to the fifo
> =A0layer so it can just do something unusual. =A0It can determine whether
> =A0the pipe is a fifo without passing around flags (the flag should be
> =A0in pipe_state).
> - My version is missing the FREAD and FWRITE checks. =A0These seem to be
> =A0necessary, but I think they don't belong at this level. =A0Also, the
> =A0error handling for them seems quite broken (nonexistent). =A0I think
> =A0POLLERR is supposed to be returned for attempts to poll for an
> =A0impossible condition, but the FREAD and FWRITE checks give a return
> =A0of 0. =A0And returning 0 is much worse than returning success, since
> =A0it will cause at least poll() to block() when it should return,
> =A0Here is the commit that added these checks:
>
> % ----------------------------
> % revision 1.118
> % date: 2005/09/12 10:16:18; =A0author: rwatson; =A0state: Exp; =A0lines:=
 +2 -2
> % Only poll the fifo for read events if the fifo is attached to a readabl=
e
> % file descriptor. =A0Otherwise, the read end of a fifo might return that=
 it
> % is writable (which it isn't).
>
> But it should return (with POLLERR). =A0This is an error condition and
> should be detected.
>
> POSIX is fuzzy about this. =A0It only says that POLLERR is for when an er=
ror
> occurred. =A0It defines the POLLNVAL error clearly as meaning that the fd
> is invalid. =A0Well that is not so clear. =A0A non-open fd is clearly inv=
alid.
> This is handled in upper layers. =A0Polling for a direction that can't wo=
rk
> can be considered as an invalid fd too, unless "invalid" has its technica=
l
> meaning. =A0Linux-2.6.10 sets POLLERR for reading from a pipe or fifo wit=
h
> no readers, and has an XXX comment saying that most Unices don't do this
> for fifos. =A0This seems wrong to me, and FreeBSD doesn't do it for any o=
f
> pipes, fifos or sockets. =A0But for pipes, there is tricky EOF handling
> associated with this condition. =A0I can't see anywhere where Linux gives
> this based on the open mode.
>
> % % Only poll the fifo for write events if the fifo attached to a writabl=
e
> % file descriptor. =A0Otherwise, the write end of a fifo might return tha=
t
> % it is readable (which it isn't).
>
> Seems to be necessary too. =A0I can't see anywhere where Linux returns
> POLLERR for i/o errors or unwritable files.
>
> % % In the event that a file is FREAD|FWRITE (which is allowed by POSIX, =
but
> % has undefined behavior), we poll for both.
> % % MFC after: =A03 days
> % ----------------------------
>
> select() is interestingly different than poll(). =A0It can't return POLLE=
RR.
> Thus, the old broken behaviour gave the best close to possible behaviour
> for select() at the usual level. =A0The POLLERR's should make it return
> success, and the false successes in the kernel would have done the same.
> Only cases where there were no false successes in the kernel were broken.
>
>
> I don't like defaults set by initializations in declarations 'revents
> =3D 0'. =A0Both the default and the return of 0 here seem to be wrong.
> This is an error condition, so I think POLLERR should be returned, as
> about. =A0Otherwise, poll() will probably block. =A0And the block is not
> just transient, at least in the above since the error condition can
> never go away. =A0You will only be saved from blocking forever if there
> is success on some other file descriptor or event.
>
> % =A0#endif
> % - =A0 =A0 if (events & (POLLIN | POLLRDNORM))
> % - =A0 =A0 =A0 =A0 =A0 =A0 if ((rpipe->pipe_state & PIPE_DIRECTW) ||
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (rpipe->pipe_buffer.cnt > 0))
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D events & (POLLIN=
 | POLLRDNORM);
> % + =A0 =A0 if (fp->f_flag & FREAD) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 if (events & (POLLIN | POLLRDNORM))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((rpipe->pipe_state & PIPE=
_DIRECTW) ||
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (rpipe->pipe_buffer.c=
nt > 0))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D =
events & (POLLIN | POLLRDNORM);
>
> The change in fifos_vnops.c was done cleanly by adding the FREAD check
> to the events mask check. =A0With fifos now polled here, it is needed
> (modulo bugs) here too. =A0But here it makes the important changes for
> fifos, if any, unreadable by indenting everything.
>
> % - =A0 =A0 if (events & (POLLOUT | POLLWRNORM))
> % - =A0 =A0 =A0 =A0 =A0 =A0 if (wpipe->pipe_present !=3D PIPE_ACTIVE ||
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (wpipe->pipe_state & PIPE_EOF) ||
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (((wpipe->pipe_state & PIPE_DIRECTW) =
=3D=3D 0) &&
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0((wpipe->pipe_buffer.size - wpipe-=
>pipe_buffer.cnt) >=3D
> PIPE_BUF ||
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0wpipe->pipe_buffer.size =
=3D=3D 0)))
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D events & (POLLOU=
T | POLLWRNORM);
> % + =A0 =A0 =A0 =A0 =A0 =A0 PIPE_UNLOCK(rpipe);
> % + =A0 =A0 =A0 =A0 =A0 =A0 if (fifo_iseof(fp))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 events |=3D POLLINIGNEOF;
> % + =A0 =A0 =A0 =A0 =A0 =A0 PIPE_LOCK(rpipe);
> % % - =A0 if ((events & POLLINIGNEOF) =3D=3D 0) {
> % - =A0 =A0 =A0 =A0 =A0 =A0 if (rpipe->pipe_state & PIPE_EOF) {
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D (events & (POLLI=
N | POLLRDNORM));
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (wpipe->pipe_present !=3D =
PIPE_ACTIVE ||
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (wpipe->pipe_state & =
PIPE_EOF))
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D =
POLLHUP;
> % + =A0 =A0 =A0 =A0 =A0 =A0 if ((events & POLLINIGNEOF) =3D=3D 0) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rpipe->pipe_state & PIPE_=
EOF) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D =
(events & (POLLIN | POLLRDNORM));
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (wpipe->pi=
pe_present !=3D PIPE_ACTIVE ||
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (wpip=
e->pipe_state & PIPE_EOF))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 revents |=3D POLLHUP;
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> % =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> % =A0 =A0 =A0 }
> % + =A0 =A0 if (fp->f_flag & FWRITE)
> % + =A0 =A0 =A0 =A0 =A0 =A0 if (events & (POLLOUT | POLLWRNORM))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (wpipe->pipe_present !=3D =
PIPE_ACTIVE ||
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (wpipe->pipe_state & =
PIPE_EOF) || % +
> =A0 =A0 =A0 =A0 =A0 =A0 (((wpipe->pipe_state & PIPE_DIRECTW) =3D=3D 0) &&
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ((wpipe->pipe_buffer.size - w=
pipe->pipe_buffer.cnt)
>>=3D
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PIPE_BUF || w=
pipe->pipe_buffer.size =3D=3D 0)))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D =
events & (POLLOUT | POLLWRNORM);
> % % =A0 =A0 if (revents =3D=3D 0) {
> % - =A0 =A0 =A0 =A0 =A0 =A0 if (events & (POLLIN | POLLRDNORM)) {
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 selrecord(td, &rpipe->pipe_se=
l);
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (SEL_WAITING(&rpipe->pipe_=
sel))
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rpipe->pipe_s=
tate |=3D PIPE_SEL;
> % - =A0 =A0 =A0 =A0 =A0 =A0 }
> % + =A0 =A0 =A0 =A0 =A0 =A0 if (fp->f_flag & FREAD)
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (events & (POLLIN | POLLRD=
NORM)) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 selrecord(td,=
 &rpipe->pipe_sel);
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (SEL_WAITI=
NG(&rpipe->pipe_sel))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 rpipe->pipe_state |=3D PIPE_SEL;
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> % % - =A0 =A0 =A0 =A0 =A0 if (events & (POLLOUT | POLLWRNORM)) {
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 selrecord(td, &wpipe->pipe_se=
l);
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (SEL_WAITING(&wpipe->pipe_=
sel))
> % - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 wpipe->pipe_s=
tate |=3D PIPE_SEL;
> % - =A0 =A0 =A0 =A0 =A0 =A0 }
> % + =A0 =A0 =A0 =A0 =A0 =A0 if (fp->f_flag & FWRITE)
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (events & (POLLOUT | POLLW=
RNORM)) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 selrecord(td,=
 &wpipe->pipe_sel);
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (SEL_WAITI=
NG(&wpipe->pipe_sel))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 wpipe->pipe_state |=3D PIPE_SEL;
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> % =A0 =A0 =A0 }
> % -#ifdef MAC
> % -locked_error:
> % -#endif
> % =A0 =A0 =A0 PIPE_UNLOCK(rpipe);
> % % =A0 =A0 return (revents);
>
> It seems that not much really changed here. =A0To avoid indentation and
> fix bugs, the FREAD and FWRITE checks should be done up front. =A0I think
> they can be done before locking and mac checking. =A0Something like:
>
> =A0 =A0 =A0 =A0if ((fp->f_flag & FREAD) && (events & (POLLIN | POLLRDNORM=
))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (POLLERR);
> =A0 =A0 =A0 =A0if ((fp->f_flag & FWRITE) && (events & (POLLOUT | POLLWRNO=
RM))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (POLLERR);
> =A0 =A0 =A0 =A0if (events & POLLINIGNEOF)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (POLLER); =A0 =A0 =A0 =A0/* try to =
kill this too */
>
> Since the diff for pipe_poll() was unreadable, here it is again with
> the old lines removed. =A0A few more problems are now obvious:
>
> % @@ -1326,58 +1549,66 @@ pipe_poll(fp, events, active_cred, td)
> % =A0 =A0 =A0 struct ucred *active_cred;
> % =A0 =A0 =A0 struct thread *td;
> % =A0{
> % + =A0 =A0 struct pipeinfo *pip =3D fp->f_data;
> % + =A0 =A0 struct pipe *rpipe;
> % =A0 =A0 =A0 struct pipe *wpipe;
> % =A0 =A0 =A0 int revents =3D 0;
> % =A0#ifdef MAC
> % =A0 =A0 =A0 int error;
> % =A0#endif
> % % + =A0 rpipe =3D pip->pi_rpipe;
> % + =A0 =A0 wpipe =3D pip->pi_wpipe->pipe_peer;
> % =A0 =A0 =A0 PIPE_LOCK(rpipe);
> % =A0#ifdef MAC
> % =A0 =A0 =A0 error =3D mac_pipe_check_poll(active_cred, rpipe->pipe_pair=
);
> % =A0 =A0 =A0 if (error)
> % + =A0 =A0 =A0 =A0 =A0 =A0 return (0);
> % =A0#endif
> % + =A0 =A0 if (fp->f_flag & FREAD) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 if (events & (POLLIN | POLLRDNORM))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((rpipe->pipe_state & PIPE=
_DIRECTW) ||
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (rpipe->pipe_buffer.c=
nt > 0))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D =
events & (POLLIN | POLLRDNORM);
> %
>
> This style bug (extra blank line) was common in old code. =A0It helps mak=
e
> the diffs unreadable too.
>


>
> % % + =A0 =A0 =A0 =A0 =A0 if ((events & POLLINIGNEOF) =3D=3D 0) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rpipe->pipe_state & PIPE_=
EOF) {
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 revents |=3D =
(events & (POLLIN | POLLRDNORM));
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (wpipe->pi=
pe_present !=3D PIPE_ACTIVE ||
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (wpip=
e->pipe_state & PIPE_EOF))
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 revents |=3D POLLHUP;
> % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>
> This is old code, reindented. =A0It was not needed, since it used to
> just check for the POLLINIGNEOF mistake in the user events. =A0Now it
> is needed to give the modified (POLLINIGNEOF) semantics from the kernel
> flag for fifos. =A0It is much uglier than the corresponding code in the
> old fifo_poll_f(). =A0That begins with putting the relevant user events
> in levents. =A0So that it doesn't have to repeat the long mask expression=
s.
> Well, that's about the limits of the cleanups. =A0Something like the
> above is still needed to give the semantics change.
>
> The socket layer still has code that corresponds exactly to the above.
> It is now not needed, since it now only supports the POLLINIGNEOF
> mistake in the user events. =A0One copy of this code is bad enough.
>

It seems to me that your concerns aren't related to the patch.
I'll try to address them when the patch will be into the tree,

--
Gianni

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq0922=y20Bo2ekp82DTv2BTqD%2BbJEYppK0nkbkef-9CuKdA>