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. I 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) > % ... > % + KASSERT(fp->f_vnode != NULL, ("fifo_iseof: no vnode info.")); > % + KASSERT(fp->f_vnode->v_fifoinfo != NULL, ("fifo_iseof: no > fifoinfo.")); > > > I wonder if you can assert that some suitable lock is held here. > > % + fip = fp->f_vnode->v_fifoinfo; > % + return (fp->f_seqcount == fip->fi_wgen); > % } > % ... 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) > % break; > % % case FIONREAD: > % + > % + /* > % + * FIONREAD will return 0 for non-readable descriptors, and > % + * the results of FIONREAD on the read pipe for readable > % + * descriptors. > % + */ > % + if (!(fp->f_flag & FREAD)) { > % + *(int *)data = 0; > % + PIPE_UNLOCK(mpipe); > % + return (0); > % + } > % if (mpipe->pipe_state & PIPE_DIRECTW) > % *(int *)data = mpipe->pipe_map.cnt; > % 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) > % int error; > % #endif > % % - wpipe = rpipe->pipe_peer; > % + wpipe = PIPE_PEER(rpipe); > % PIPE_LOCK(rpipe); > % #ifdef MAC > % error = mac_pipe_check_poll(active_cred, rpipe->pipe_pair); > % if (error) > % goto locked_error; > % #endif > % - if (events & (POLLIN | POLLRDNORM)) > % - if ((rpipe->pipe_state & PIPE_DIRECTW) || > % - (rpipe->pipe_buffer.cnt > 0)) > % - revents |= events & (POLLIN | POLLRDNORM); > % - > % - if (events & (POLLOUT | POLLWRNORM)) > % - if (wpipe->pipe_present != PIPE_ACTIVE || > % - (wpipe->pipe_state & PIPE_EOF) || > % - (((wpipe->pipe_state & PIPE_DIRECTW) == 0) && > % - ((wpipe->pipe_buffer.size - wpipe->pipe_buffer.cnt) >= > PIPE_BUF || > % - wpipe->pipe_buffer.size == 0))) > % - revents |= events & (POLLOUT | POLLWRNORM); > % - > % - if ((events & POLLINIGNEOF) == 0) { > % - if (rpipe->pipe_state & PIPE_EOF) { > % - revents |= (events & (POLLIN | POLLRDNORM)); > % - if (wpipe->pipe_present != PIPE_ACTIVE || > % - (wpipe->pipe_state & PIPE_EOF)) > % - revents |= POLLHUP; > % + if (fp->f_flag & FREAD) { > % + if (events & (POLLIN | POLLRDNORM)) > % + if ((rpipe->pipe_state & PIPE_DIRECTW) || > % + (rpipe->pipe_buffer.cnt > 0)) > % + revents |= events & (POLLIN | POLLRDNORM); > % + > > This part of the patch is still unreadable since it indents everything to > add FREAD and FWRITE checks. I 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. That involved using && instead of a nested if. In > the old code, everything was under the combined && if. Now there are > 3 nested ifs. This logic change seems to be non-null and give bugs (1). > > % + if (rpipe->pipe_state & PIPE_NAMED) > % + if (fifo_iseof(fp)) > > Here the (unrelated) nested if is just a style bug. It gives verboseness > and extra indentation. > > % + events |= POLLINIGNEOF; > % + > > (1) In the old code, POLLINIGNEOF was only set here if we are polling for > some input condition (and this is valid). Now it is set if the file is > just open for input. Either way could be right, but this is a change. If > 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. There 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>