Date: Sun, 7 Mar 2004 05:12:17 -0800 From: Luigi Rizzo <rizzo@icir.org> To: Johan Karlsson <johan@freebsd.org> Cc: ipfw@freebsd.org Subject: Re: where do %j/uintmax_t stand in terms of standards? [WAS: Re: WARNS cleanup for ipfw Message-ID: <20040307051216.A74559@xorpc.icir.org> In-Reply-To: <20040307113008.GC64109@numeri.campus.luth.se>; from johan@freebsd.org on Sun, Mar 07, 2004 at 12:30:08PM %2B0100 References: <20040306111922.GA64109@numeri.campus.luth.se> <20040306082625.B34490@xorpc.icir.org> <20040306173219.GB64109@numeri.campus.luth.se> <20040306212233.A56351@xorpc.icir.org> <20040307113008.GC64109@numeri.campus.luth.se>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Mar 07, 2004 at 12:30:08PM +0100, Johan Karlsson wrote:
...
> Ok, how about the attached patch then? It takes care of all printf
> related warnings on -current.
not there yet, sorry...
No offense, but I think that rather than rushing for a commit you
should wait a few days to get some feedback from people using 64-bit
platforms (e.g. try to post to -sparc or -alpha, or ask some of
the people involved with 64-bit development), and also have a look
at how other system utilities deal with similar things (64-bit
counters and possible alignment problems -- what is the preferred
way to print out things, "%qu" or "%llu" ? I understand that
ipfw2.c does a mix of both things, i just have no idea which one is
better except that "unsigned long long" is a lot longer to
write than "u_quad_t" so that might favour "%qu" ?).
In any case, it's a weekend, give people a bit of time to read and
think about solutions.
This is muddy ground, I and possibly others have burned our fingers
by making the wrong assumptions. A clean and silent compile
can still cause the code to dump core on certain systems due to
alignment problems.
E.g. I strongly suspect that, on systems with aligmnent issues,
many of the places where you cast values to (unsigned long long)
would be a lot safer by using align_uint64() (I believe the current
code _is_ safe because of the way the pointer to list_pipes() and
print_flowset_parms() are constructed, but all this is very fragile,
because it relies on the userland passing down a suitably aligned
buffer, which is not specified anywhere in the ipfw ABI,
If we are going to touch this code we better provide a good fix
than a bandaid.)
cheers
luigi
> I do not have a -stable machine at the moment so I have not done any
> compile testing for -stable. If you agree to this patch, please commit
> it or let me know if I should.
>
> take care
> /Johan K
>
> --
> Johan Karlsson mailto:johan@FreeBSD.org
> Index: sbin/ipfw/Makefile
> ===================================================================
> RCS file: /home/ncvs/src/sbin/ipfw/Makefile,v
> retrieving revision 1.12
> diff -u -r1.12 Makefile
> --- sbin/ipfw/Makefile 11 Jul 2002 17:33:37 -0000 1.12
> +++ sbin/ipfw/Makefile 5 Mar 2004 22:06:10 -0000
> @@ -2,7 +2,7 @@
>
> PROG= ipfw
> SRCS= ipfw2.c
> -WARNS?= 0
> +WARNS?= 2
> MAN= ipfw.8
>
> .include <bsd.prog.mk>
> Index: sbin/ipfw/ipfw2.c
> ===================================================================
> RCS file: /home/ncvs/src/sbin/ipfw/ipfw2.c,v
> retrieving revision 1.45
> diff -u -r1.45 ipfw2.c
> --- sbin/ipfw/ipfw2.c 24 Jan 2004 19:20:09 -0000 1.45
> +++ sbin/ipfw/ipfw2.c 7 Mar 2004 11:12:34 -0000
> @@ -352,12 +352,12 @@
> { NULL, 0 } /* terminator */
> };
>
> -static __inline uint64_t
> +static unsigned long long
> align_uint64(uint64_t *pll) {
> uint64_t ret;
>
> bcopy (pll, &ret, sizeof(ret));
> - return ret;
> + return (unsigned long long)ret;
> };
>
> /*
> @@ -1423,12 +1423,14 @@
> ina.s_addr = htonl(q[l].id.dst_ip);
> printf("%15s/%-5d ",
> inet_ntoa(ina), q[l].id.dst_port);
> - printf("%4qu %8qu %2u %4u %3u\n",
> - q[l].tot_pkts, q[l].tot_bytes,
> + printf("%4llu %8llu %2u %4u %3u\n",
> + (unsigned long long)q[l].tot_pkts,
> + (unsigned long long)q[l].tot_bytes,
> q[l].len, q[l].len_bytes, q[l].drops);
> if (verbose)
> - printf(" S %20qd F %20qd\n",
> - q[l].S, q[l].F);
> + printf(" S %20llu F %20llu\n",
> + (unsigned long long)q[l].S,
> + (unsigned long long)q[l].F);
> }
> }
>
> @@ -1517,7 +1519,8 @@
> p->pipe_nr, buf, p->delay);
> print_flowset_parms(&(p->fs), prefix);
> if (verbose)
> - printf(" V %20qd\n", p->V >> MY_M);
> + printf(" V %20llu\n",
> + (unsigned long long)p->V >> MY_M);
>
> q = (struct dn_flow_queue *)(p+1);
> list_queues(&(p->fs), q);
> _______________________________________________
> freebsd-ipfw@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-ipfw
> To unsubscribe, send any mail to "freebsd-ipfw-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040307051216.A74559>