From owner-cvs-all  Mon Oct 29  6:36:48 2001
Delivered-To: cvs-all@freebsd.org
Received: from flood.ping.uio.no (flood.ping.uio.no [129.240.78.31])
	by hub.freebsd.org (Postfix) with ESMTP
	id BCF8F37B403; Mon, 29 Oct 2001 06:36:42 -0800 (PST)
Received: by flood.ping.uio.no (Postfix, from userid 2602)
	id 1CF1B14C2E; Mon, 29 Oct 2001 15:36:39 +0100 (CET)
X-URL: http://www.ofug.org/~des/
X-Disclaimer: The views expressed in this message do not necessarily
  coincide with those of any organisation or company with
  which I am or have been affiliated.
To: "Andrew R. Reiter" <arr@watson.org>
Cc: Luigi Rizzo <rizzo@aciri.org>,
	Josef Karthauser <joe@FreeBSD.org>, cvs-committers@FreeBSD.org,
	cvs-all@FreeBSD.org
Subject: Re: ipfw.c -- (was: cvs commit: src/sys/netinet ip_fw.h)
References: <Pine.NEB.3.96L.1011029010151.33440A-100000@fledge.watson.org>
From: Dag-Erling Smorgrav <des@ofug.org>
Date: 29 Oct 2001 15:36:38 +0100
In-Reply-To: <Pine.NEB.3.96L.1011029010151.33440A-100000@fledge.watson.org>
Message-ID: <xzp4roiqzrd.fsf@flood.ping.uio.no>
Lines: 55
User-Agent: Gnus/5.0808 (Gnus v5.8.8) Emacs/20.7
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Sender: owner-cvs-all@FreeBSD.ORG
Precedence: bulk
List-ID: <cvs-all.FreeBSD.ORG>
List-Archive: <http://docs.freebsd.org/mail/> (Web Archive)
List-Help: <mailto:majordomo@FreeBSD.ORG?subject=help> (List Instructions)
List-Subscribe: <mailto:majordomo@FreeBSD.ORG?subject=subscribe%20cvs-all>
List-Unsubscribe: <mailto:majordomo@FreeBSD.ORG?subject=unsubscribe%20cvs-all>
X-Loop: FreeBSD.ORG

"Andrew R. Reiter" <arr@watson.org> writes:
>         if (chain->fw_flg & IP_FW_F_RND_MATCH) {
>                 double d = 1.0 * chain->dont_match_prob;
>                 d = 1 - (d / 0x7fffffff);
>                 printf("prob %f ", d);
>         }

There is no need for a temporary variable here.  The probability
computation can be greatly simplified.  BTW, the format string should
specify the number of decimals (e.g. "%.3f").

        if (chain->fw_flg & IP_FW_F_RND_MATCH)
                printf("prob %.3f ",
                    1.0 - chain->dont_match_prob / 0x7fffffff);

Also, the use of the hard-coded value 0x7fffffff looks suspicious.  It
looks like a scaling factor, which means it's probably used in several
other places both in the kernel and in userland; it should at the very
least be a symbolic constant (e.g. PROB_SCALE_FACTOR); at best, there
should be a preprocessor macro that converts the dont_match_prob value
to a double in the appropriate range.

>         if (do_time) {
>                 if (chain->timestamp) {
>                         char timestr[30];
>                         strcpy(timestr, ctime((time_t *)&chain->timestamp));
>                         *strchr(timestr, '\n') = '\0';
>                         printf("%s ", timestr);
>                 } else {
>                         printf("                         ");
>                 }
>         }

This code should probably use strftime() instead of ctime(), and
format the timestamp in ISO8601 format (YYYY-MM-DD HH:MM:SS).  It also
makes potentially unsound assumptions about the length of the text
generated by ctime().

Both printf()s can be written more intelligently (and more obviously
correct) using width and precision specifiers.

        if (do_time) {
                char timestr[20];

                if (chain->timestamp)
                        strftime(timestr, sizeof timestr, "%Y-%m-%d %H:%M:%S",
                            localtime(chain->timestamp));
                else
                        timestr[0] = '\0';
                printf("%-20.20s", timestr);
        }

DES
-- 
Dag-Erling Smorgrav - des@ofug.org

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message