Date: Fri, 17 Feb 2012 02:59:23 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Giovanni Trematerra <gianni@FreeBSD.org> Cc: flo@FreeBSD.org, Kip Macy <kmacy@FreeBSD.org>, John Baldwin <jhb@FreeBSD.org>, Peter Holm <pho@FreeBSD.org>, Attilio Rao <attilio@FreeBSD.org>, Konstantin Belousov <kib@FreeBSD.org>, bde@FreeBSD.org, freebsd-arch@FreeBSD.org, jilles@FreeBSD.org Subject: Re: [LAST ROUND] fifo/pipe merge code Message-ID: <20120217015048.H3388@besplex.bde.org> 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, 8 Feb 2012, Giovanni Trematerra wrote: Sorry for the delay. > 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: > ... > 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 It looks very good now. I only see very minor style bugs :-). I didn't think about its correctness again. 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. A full conversion to the vnode.h method would be more concise and say something like: * p - pipe lock * v - vnode lock and then put these letters in comments attached more directly to the fi_* declarations. 2 places in sys_pipe.c fail to use PIPE_MTX() to hide the details. % ... % 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. % #ifdef MAC % int error; % #endif % % - wpipe = rpipe->pipe_peer; % + wpipe = PIPE_PEER(rpipe); rpipe should be initialized like wpipe here. % 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 (fp->f_flag & FREAD && 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); You changed the indentation when moving the above. It's not quite right here, but no need to change it, and some of the changes are regressions. % @@ -1361,15 +1428,22 @@ pipe_poll(fp, events, active_cred, td) % revents |= POLLHUP; % } % } % + if (fp->f_flag & FWRITE && 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) >= Now it is missing 4 spaces of indententation for the previous 2 lines... % + PIPE_BUF || wpipe->pipe_buffer.size == 0))) ... and has an extra 4 spaces for the previous 1 line. % + revents |= events & (POLLOUT | POLLWRNORM); Otherwise, this part of the patch is now readable :-). % ... % @@ -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; This file mostly has the consistently non-KNF style of initializing both rpipe and wpipe in their declarations. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120217015048.H3388>