Date: Wed, 8 Feb 2012 22:14:48 +0100 From: Giovanni Trematerra <gianni@freebsd.org> To: freebsd-arch@freebsd.org Cc: Kip Macy <kmacy@freebsd.org>, Attilio Rao <attilio@freebsd.org>, Konstantin Belousov <kib@freebsd.org>, bde@freebsd.org, jilles@freebsd.org Subject: Fwd: [LAST ROUND] fifo/pipe merge code Message-ID: <CACfq092eJHEfR3uKmCobxpS0WiM87ZH9isBMFaW18AC_-Xg6HA@mail.gmail.com> In-Reply-To: <CACfq091eHpvCbwuU-WOxiyt92YiTbRbWnPmNGGMDidzG=uq4uw@mail.gmail.com> References: <CACfq090%2BEDVjj_rRNyafiod_BBzGUzmQ9CjApZLcXWR-Ax=0uQ@mail.gmail.com> <20120208151251.J1853@besplex.bde.org> <CACfq091eHpvCbwuU-WOxiyt92YiTbRbWnPmNGGMDidzG=uq4uw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Feb 8, 2012 at 6:49 AM, Bruce Evans <brde@optusnet.com.au> wrote: > On Tue, 7 Feb 2012, Giovanni Trematerra wrote: > > > It looks quite good now. =A0I see only a few minor style bugs: Hi Bruce, Thank you to have found time to review the patch again. I updated the patch and I hope I fixed all the minor style bugs that you pointed out. here the new one: http://www.trematerra.net/patches/pipefifo_merge.4.4.patch > > % diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c > % index 94a2713..0d35877 100644 > % --- a/sys/fs/fifofs/fifo_vnops.c > % +++ b/sys/fs/fifofs/fifo_vnops.c > % ... > % +int > % +fifo_iseof(struct file *fp) > % ... > % + =A0 =A0 KASSERT(fp->f_vnode !=3D NULL, ("fifo_iseof: no vnode info.")= ); > % + =A0 =A0 KASSERT(fp->f_vnode->v_fifoinfo !=3D NULL, ("fifo_iseof: no > fifoinfo.")); > > > I wonder if you can assert that some suitable lock is held here. > > % + =A0 =A0 fip =3D fp->f_vnode->v_fifoinfo; > % + =A0 =A0 return (fp->f_seqcount =3D=3D fip->fi_wgen); > % =A0} > % ... Now I changed fifo_open to remove global fifo_mtx mutex, using PIPE_MTX, and assert to be held in fifo_iseof. I stress tested the patch with WITNESS and INVARIANT on and got no a panic and no LOR. > > % @@ -1284,6 +1348,17 @@ pipe_ioctl(fp, cmd, data, active_cred, td) > % =A0 =A0 =A0 =A0 =A0 =A0 =A0 break; > % % =A0 =A0 case FIONREAD: > % + > % + =A0 =A0 =A0 =A0 =A0 =A0 /* > % + =A0 =A0 =A0 =A0 =A0 =A0 =A0* FIONREAD will return 0 for non-readable = descriptors, and > % + =A0 =A0 =A0 =A0 =A0 =A0 =A0* the results of FIONREAD on the read pipe= for readable > % + =A0 =A0 =A0 =A0 =A0 =A0 =A0* descriptors. > % + =A0 =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 *(int *)data =3D 0; > % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PIPE_UNLOCK(mpipe); > % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return (0); > % + =A0 =A0 =A0 =A0 =A0 =A0 } > % =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (mpipe->pipe_state & PIPE_DIRECTW) > % =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *(int *)data =3D mpipe->pip= e_map.cnt; > % =A0 =A0 =A0 =A0 =A0 =A0 =A0 else I completely removed that comment now. The code is clear enough. > > Urk, now I see many major style bugs: > > % @@ -1333,47 +1408,55 @@ pipe_poll(fp, events, active_cred, td) > % =A0 =A0 =A0 int error; > % =A0#endif > % % - =A0 wpipe =3D rpipe->pipe_peer; > % + =A0 =A0 wpipe =3D PIPE_PEER(rpipe); > % =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 =A0 goto locked_error; > % =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 (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 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 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 part of the patch is still unreadable since it indents everything to > add FREAD and FWRITE checks. =A0I don't like those anyway (they should > probably produce POLLERR POLLNVAL instead of ignoring the error -- see > previous mail), but they are needed for now for compatibility in the > FIFO case (hopefully they don't break compatibility for the plain pipe > case). I tried to make this part of the patch more readable, I hope I did. Talking about returning POLLERR/POLLNVAL as I said previously, I like to work on that and on making pipe/fifo implementation more POSIX compliant just after the patch will be merge the tree. > To avoid churning this code for them now and later when the > behaviour is fixed, they should be added in a non-invasive way like the > old fifo code did. =A0That involved using && instead of a nested if. =A0I= n > the old code, everything was under the combined && if. =A0Now there are > 3 nested ifs. =A0This logic change seems to be non-null and give bugs (1)= . > > % + =A0 =A0 =A0 =A0 =A0 =A0 if (rpipe->pipe_state & PIPE_NAMED) > % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fifo_iseof(fp)) > > Here the (unrelated) nested if is just a style bug. =A0It gives verbosene= ss > and extra indentation. > > % + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 events |=3D P= OLLINIGNEOF; > % + > > (1) In the old code, POLLINIGNEOF was only set here if we are polling for > some input condition (and this is valid). =A0Now it is set if the file is > just open for input. =A0Either way could be right, but this is a change. = =A0If > we are not polling for input, we don't care about it, but POSIX requires > POLLHUP to be set in some cases, and this affects whether we return or > block, and POLLINIGNEOF interacts with this subtly. =A0There may also be > a problem not going through here to set POLLINIGNEOF in the !FREAD case. Fixed. Thank you. While I hope I didn't introduced new bugs, do you agree that at this point the patch is in such a shape that makes it committable? -- Gianni
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq092eJHEfR3uKmCobxpS0WiM87ZH9isBMFaW18AC_-Xg6HA>