Fwd: [LAST ROUND] fifo/pipe merge code

[Giovanni Trematerra]

On Wed, Feb 8, 2012 at 6:49 AM, Bruce Evans <brde at 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