Date: Mon, 24 Aug 2009 00:14:42 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Schouten <ed@80386.nl> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Julian Elischer <julian@FreeBSD.org> Subject: Re: svn commit: r196451 - head/sys/netinet/ipfw Message-ID: <20090823230300.Q38728@delplex.bde.org> In-Reply-To: <20090823080940.GT1292@hoeg.nl> References: <200908230759.n7N7xS6g051165@svn.freebsd.org> <20090823080940.GT1292@hoeg.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 23 Aug 2009, Ed Schouten wrote:
> * Julian Elischer <julian@FreeBSD.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090823230300.Q38728>