Date: Sun, 15 Aug 2004 22:20:55 -0700 From: John-Mark Gurney <gurney_j@resnet.uoregon.edu> To: Brian Fundakowski Feldman <green@FreeBSD.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/cam/scsi scsi_target.c src/sys/dev/mii mii.c src/sys/fs/fifofs fifo_vnops.c src/sys/gnu/ext2fs ext2_vnops.c src/sys/kern init_main.c kern_conf.c kern_descrip.c kern_event.c kern_exec.c kern_exit.c kern_fork.c kern_sig.c sys_pipe.c tty.c ... Message-ID: <20040816052054.GR991@funkthat.com> In-Reply-To: <20040816023851.GC3026@green.homeunix.org> References: <200408150624.i7F6OhhR074096@repoman.freebsd.org> <20040816014244.GB3026@green.homeunix.org> <20040816015108.GQ991@funkthat.com> <20040816023851.GC3026@green.homeunix.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Brian Fundakowski Feldman wrote this message on Sun, Aug 15, 2004 at 22:38 -0400:
> On Sun, Aug 15, 2004 at 06:51:08PM -0700, John-Mark Gurney wrote:
> > sure, I'd like a quick peak at the patch though (if it takes me more than
> > a day, go ahead and commit).
>
> I'd very much like you to look it over. I haven't tested it, just made
> the stylistic changes. If it were more complete (i.e. satisfy bde),
> every spurious blank line (that is, all of them inside functions which
> are not between variable declarations and code) would be gone, too, but
> that can be kind of harsh.
As was pointed out... blank lines at the top of functions w/o variables
are mandated by style(9)...
> - * internal flag indicating registration done by kernel
> + * Internal flag indicating registration done by kernel
Forgot the period...
> - if ((kev->flags & EV_ADD) == EV_ADD && kqueue_expand(kq, fops,
> - kev->ident, 0) != 0) {
> - /* unlock and try again */
> - FILEDESC_UNLOCK(fdp);
> - fdrop(fp, td);
> - fp = NULL;
> - error = kqueue_expand(kq, fops, kev->ident, waitok);
> - if (error)
> - goto done;
> - goto findkn;
> + if (kev->flags & EV_ADD) {
> + error = kqueue_expand(kq, fops, kev->ident, 0);
> + if (error) {
> + /* Unlock and try again */
> + FILEDESC_UNLOCK(fdp);
> + fdrop(fp, td);
> + fp = NULL;
> + if (!waitok)
> + goto done;
> + error = kqueue_expand(kq, fops, kev->ident, 1);
> + if (error)
> + goto done;
> + goto findkn;
> + }
This one I think is gross (as in large, excessive)... error is not used
from the first call to kqueue_expand, and ends up indenting more code then
necessary... I find it easier to read the single if statement, then
breaking the logic into two seperate blocks..
there was is also a case where you sorted mflag back into the set, you
keep the variables COMPLETELY out of order.. putting mflag before fd..
at a minimum, the mflag assignment should be moved to the body, and the
variables properly sorted...
My personal feeling is that this patch is a bit excesive in the changes,
and goes beyond the standard of keeping style changes to a minimum, unless
you have additional changes to the source file...
I was doing the (var & FLAG) == FLAG to make it easier to read w/ all
the flag comparisions I was looking at...
As rwatson pointed out, deleting was misspelled, which you missed in
your sweep...
If you can get bde to sign off on the patch completely (esspecially
introducing soo much code churn), and completely test it, then I don't
have an objection.
NOTE: I did not completely review the patch, I only got about half way
or so.
/me thinks of #bsdcode's key.
--
John-Mark Gurney Voice: +1 415 225 5579
"All that I will do, has been done, All that I have, has not."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040816052054.GR991>