[LAST ROUND] fifo/pipe merge code

[Giovanni Trematerra]

On Thu, Feb 16, 2012 at 4:59 PM, Bruce Evans <brde at optusnet.com.au> wrote:
> On Wed, 8 Feb 2012, Giovanni Trematerra wrote:
>
> Sorry for the delay.

No problem, thanks to you for your time.

> It looks very good now.  I only see very minor style bugs :-).  I
> didn't think about its correctness again.

Hopefully this is the last version that fix the remaining style bugs:

http://www.trematerra.net/patches/pipefifo_merge.4.5.patch

I would appreciate it if someone were to step up and commit this patch.

Thank you.

Below some comments of mine.

>
> diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c
> index 94a2713..729e49c 100644
> --- a/sys/fs/fifofs/fifo_vnops.c
> +++ b/sys/fs/fifofs/fifo_vnops.c
> % ...
> %  /*
> %   * This structure is associated with the FIFO vnode and stores
> %   * the state associated with the FIFO.
> %   * Notes about locking:
> % - *   - fi_readsock and fi_writesock are invariant since init time.
> % + *   - fi_pipe is invariant since init time.
> %   *   - fi_readers and fi_writers are vnode lock protected.
> % - *   - fi_wgen is fif_mtx lock protected.
> % + *   - fi_wgen is PIPE_MTX lock protected.
>
> Can you find a better name than PIPE_MTX here and in a comment later?
> PIPE_MTX is the name of the macro that is supposed to hide the details
> of the lock, starting with the lock's name.  Using it in comments is
> like unhiding the details.  Mutexes aren't the same as locks, and vnode.h is
> more careful to distinguish them.  I adapted the following
> better wording from vnode.h.
>
>  *   - fi_readers and fi_writers are protected by the vnode lock.
>  *   - fi_wgen is protected by the pipe mutex.

I used this wording. Thanks for the suggestion.

>
> 2 places in sys_pipe.c fail to use PIPE_MTX() to hide the details.
>

I'm going to fix this and other style bugs not related to my patch with a
different one.

>
> % ...
> % diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c
> % index 9edcb74..4a1d8f3 100644
>
> % --- a/sys/kern/sys_pipe.c
> % +++ b/sys/kern/sys_pipe.c
> %  /*
> %   * Use this define if you want to disable *fancy* VM things.  Expect an
> %   * approx 30% decrease in transfer rate.  This could be useful for
> % @@ -1329,29 +1397,28 @@ pipe_poll(fp, events, active_cred, td)
> %       struct pipe *rpipe = fp->f_data;
> %       struct pipe *wpipe;
> %       int revents = 0;
> % +     int levents;
>
> Variables with the same type should be declared on 1 line if possible,
> not as for rpipe and wpipe above.  Initializations in declarations can
> make this hard to read, but are style bugs.
>
> Variables with the same type should be sorted.

fixed.

>
> Otherwise, this part of the patch is now readable :-).

The pipe_poll part of the patch should be readable now.
I hope :)

>
> % ...
> % @@ -1625,7 +1708,7 @@ filt_pipedetach(struct knote *kn)
> %  static int
> %  filt_piperead(struct knote *kn, long hint)
> %  {
> % -     struct pipe *rpipe = kn->kn_fp->f_data;
> % +     struct pipe *rpipe = (struct pipe *)kn->kn_hook;
>
> This cast shouldn't be added here or elsewhere.  kn_hook has type `void *',
> so this cast is not needed in C, unlike in C++.
>
> %       struct pipe *wpipe = rpipe->pipe_peer;
> %       int ret;

fixed.

>
> This file mostly has the consistently non-KNF style of initializing both
> rpipe and wpipe in their declarations.
>

I'll try to fix with a different patch.

--
Gianni