Date: Tue, 16 Dec 2008 10:42:37 -0800 From: Julian Elischer <julian@elischer.org> To: Robert Watson <rwatson@FreeBSD.org> Cc: svn-src-head@freebsd.org, Max Laier <max@love2party.net>, svn-src-all@freebsd.org, src-committers@freebsd.org, jhb@FreeBSD.org Subject: Re: svn commit: r186187 - head/sys/net Message-ID: <4947F69D.3040107@elischer.org> In-Reply-To: <alpine.BSF.1.10.0812161751250.75599@fledge.watson.org> References: <200812161703.mBGH3M7m042955@svn.freebsd.org> <200812161821.37686.max@love2party.net> <alpine.BSF.1.10.0812161751250.75599@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Robert Watson wrote: > > On Tue, 16 Dec 2008, Max Laier wrote: > >>> Log: >>> A few locking fixes and cleanups to pfil hook registration, >>> unregistration, and execution: >>> >>> - Add some brackets for clarity and trim a bit of vertical whitespace. >>> - Remove comments that may not contribute to clarity, such as "Lock" >>> before acquiring a lock and "Get memory" before allocating memory. >> >> These were meant as sectioning comments as pfil_add_hook grew larger >> and larger in order to allow WAITOK allocations. Should probably have >> been "/* 1. get memory */" etc. > > I used to comment a bit more gratuitously along those lines, but bde > seems to have (at least partially) broken me of the habit. I felt sure > there was a note in style(9) on not commenting on obvious things, but > the closest I found was this: > > exit(EX_OK); /* > * Avoid obvious comments such as > * "Exit 0 on success." > */ Generally I want more comments not less.. What is obvious to the writer may not be obvious to the reader, or, the writer after 12 months. BDE, while often correct goes to far sometimes.. :-) > > I think, generally, that I agree with the observation that code comments > should contribute to greater understanding of the code by summarizing > complex code in a simple way, or lending insight to non-obvious > implications of code, and that simply mirroring of code behavior at a > low level in comments doesn't contribute a great deal. or allowing the reader to understand why a particular approach was taken instead of another (perhaps just as obvious) one. > > Speaking of PFIL_WAITOK, I actually had a pfil_flags-related question > for you: I notice that pfil on NetBSD saves and propagates pfil_flags > from handler to the packet_filter_hook, but on FreeBSD, the flags field > is unused. Is that intentional? If so, I was thinking of putting a /* > Unused. */ comment in the definition of packet_filter_hook. > > Also, I notice PFIL_TYPE_IFNET is defined but not implemented -- did you > plan to implement it at some point? > > [...] >> Again, this lock also ensures that we see all the changes done in >> other threads to the pfil head recently - otherwise we might leak a >> hook function pointer or even fault due to list inconsistencies. >> Again, add/del_hook don't interact with the LIST_LOCK. > > We may run into similar problems with VIMAGE destructors now that we are > interested in tearing down network stacks. Again, perhaps jhb can opine > some. thar be dragons... > > Robert N M Watson > Computer Laboratory > University of Cambridge
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4947F69D.3040107>