From owner-svn-src-all@FreeBSD.ORG Sun Aug 23 14:14:56 2009 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id ABEF6106568C; Sun, 23 Aug 2009 14:14:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail01.syd.optusnet.com.au (mail01.syd.optusnet.com.au [211.29.132.182]) by mx1.freebsd.org (Postfix) with ESMTP id 41C0E8FC0C; Sun, 23 Aug 2009 14:14:56 +0000 (UTC) Received: from c122-106-152-1.carlnfd1.nsw.optusnet.com.au (c122-106-152-1.carlnfd1.nsw.optusnet.com.au [122.106.152.1]) by mail01.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n7NEEg7W027211 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 24 Aug 2009 00:14:45 +1000 Date: Mon, 24 Aug 2009 00:14:42 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Ed Schouten In-Reply-To: <20090823080940.GT1292@hoeg.nl> Message-ID: <20090823230300.Q38728@delplex.bde.org> References: <200908230759.n7N7xS6g051165@svn.freebsd.org> <20090823080940.GT1292@hoeg.nl> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Julian Elischer Subject: Re: svn commit: r196451 - head/sys/netinet/ipfw X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 Aug 2009 14:14:56 -0000 On Sun, 23 Aug 2009, Ed Schouten wrote: > * Julian Elischer wrote: >> + * (now that we a re guaranteed of success). > ^^^^ > > ;-) At least 5 more commits are needed at this rate to fix the comment. % /* % * Other things that are only done the first time. ^ (1) ^ (2) % * (now that we a re guaranteed of success). ^^^^ (3) ^^ (4) % */ (1) remove syntax error ("." in the middle of a pseudo-sentence (2) fix line break (don't format for 60-column terminals) (3) as above (4) remove syntax error (5) either remove the comment, or change it so that it says something that is useful or at least correct. This is an especially initial init function, so of course it does things that are done the first time, and other comments in it describe the onlyness of this better. This function always succeeds, so its success is guaranteed throughout the function, not just in the lines following this comment. 100 or so commits at this rate would be needed to fix other style bugs in this function: % % % 3 extra blank lines before the function. % /**************** Non-KNF block comment. % * Stuff that must be initialised only on boot or module load Strange (English?) spelling of "initialized". Sentence not terminated with a ".". % */ % int % ipfw_init(void) % { % int error = 0; This variable is not really used. Initialization in declaration. This mainly obfuscates the non-use of the variable. % % ipfw_dyn_rule_zone = uma_zcreate("IPFW dynamic rule", % sizeof(ipfw_dyn_rule), NULL, NULL, NULL, NULL, % UMA_ALIGN_PTR, 0); Bogus line splitting for the second split (the split is long before needed, and in fact is not needed). % Extra blank line % IPFW_DYN_LOCK_INIT(); Minor obfuscation. This macro is only used once. % /* % * Only print out this stuff the first time around, % * when called from the sysinit code. % */ Missing blank line before the comment. KNF doesn't really allow this either, but if there are to be blank lines in functions then they belong before blocks of code (and all blocks of code should begin with a comment). Comment adds less than nothing. This is an especially initial init function, so of course it does stuff that is only done the first time, not just this printf, and this is better described earlier. % printf("ipfw2 " % #ifdef INET6 % "(+ipv6) " % #endif % "initialized, divert %s, nat %s, " % "rule-based forwarding " % #ifdef IPFIREWALL_FORWARD % "enabled, " % #else % "disabled, " % #endif % "default to %s, logging ", % #ifdef IPDIVERT % "enabled", % #else % "loadable", % #endif % #ifdef IPFIREWALL_NAT % "enabled", % #else % "loadable", % #endif % default_to_accept ? "accept" : "deny"); I hate using C90 string concatenation to obfuscate messages, but don't see anything better here. % Extra blank line. This would normally be correct before a block comment, but here the block comment breaks the block -- to the extent that the previous comment is useful, its scope doesn't end here -- the "block" consistes of all the printfs. % /* % * Note: V_xxx variables can be accessed here but the iattach() % * may not have been called yet for the VIMGE case. % * Tuneables will have been processed. % */ Grammar error "the iattach()". Should be plain iattach in KNF. I prefer iattach(). Could be "the iattach() function to be as verbose as man pages. Semantic error "may" (should be "might"). Spelling error "VIMGE". Possible spelling error "Tuneables". ispell gives only tunable, "tune able" and "tune-able". % if (V_fw_verbose == 0) % printf("disabled\n"); % else if (V_verbose_limit == 0) % printf("unlimited\n"); % else % printf("limited to %d packets/entry by default\n", % V_verbose_limit); This is an extension of the previous printed, with a completely different style. It uses separate printfs instead of combining everything using complicated logic involving ifdefs and the ?: operator. This is probably better. With the second style, the newline should be in a separate printf too. The printfs are too verbose (especially the last one), so they might give too-long lines in output, but a single line is best for a log message. % % /* % * Other things that are only done the first time. % * (now that we a re guaranteed of success). % */ See above. % ip_fw_ctl_ptr = ipfw_ctl; % ip_fw_chk_ptr = ipfw_chk; Missing blank line. The above comment and its block end before the return statement. % return (error); This is an obfuscation of "return (0);". Old versions of this function sometimes failed, so the "(now we a re cuaranteed success)" (sic) comment would have applied to them, but they didn't have the comment. They needed the `error' variable, but they were missing the obfuscatory initialization of it to 0 and the obfuscatory return of it (they just returned 0 here). % } Bruce