Date: Sat, 13 Feb 2010 00:07:54 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: =?X-UNKNOWN?Q?Ermal_Lu=E7i?= <eri@FreeBSD.org> Cc: src-committers@FreeBSD.org, svn-src-user@FreeBSD.org Subject: Re: svn commit: r203777 - in user/eri/pf45/head: contrib/pf/pfctl sys/contrib/pf/net Message-ID: <20100212231841.Q89449@delplex.bde.org> In-Reply-To: <201002110955.o1B9tklJ038602@svn.freebsd.org> References: <201002110955.o1B9tklJ038602@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--0-619413657-1265980074=:89449
Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
On Thu, 11 Feb 2010, Ermal Lu=E7i wrote:
> Log:
> Cast to proper type.
>
> Requested-by: Jilles Tjoelker
Isn't the proper type [u]intmax_t (provided the format is also proper)?
> Modified: user/eri/pf45/head/contrib/pf/pfctl/parse.y
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- user/eri/pf45/head/contrib/pf/pfctl/parse.y=09Thu Feb 11 08:50:21 201=
0=09(r203776)
> +++ user/eri/pf45/head/contrib/pf/pfctl/parse.y=09Thu Feb 11 09:55:45 201=
0=09(r203777)
> @@ -709,7 +709,7 @@ varstring=09: numberstring varstring =09=09{
>
> numberstring=09: NUMBER=09=09=09=09{
> =09=09=09char=09*s;
> -=09=09=09if (asprintf(&s, "%lld", (int64_t)$1) =3D=3D -1) {
> +=09=09=09if (asprintf(&s, "%lld", (long long)$1) =3D=3D -1) {
The long long type is always improper, as is the %lld format that goes
with it.
> @@ -2859,7 +2859,7 @@ host=09=09: STRING=09=09=09{
>
> =09=09=09/* ie. for 10/8 parsing */
> #ifdef __FreeBSD__
> -=09=09=09if (asprintf(&buf, "%lld/%lld", (int64_t)$1, (int64_t)$3) =3D=
=3D -1)
> +=09=09=09if (asprintf(&buf, "%lld/%lld", (long long)$1, (long long)$3) =
=3D=3D -1)
> #else
> =09=09=09if (asprintf(&buf, "%lld/%lld", $1, $3) =3D=3D -1)
> #endif
Why messy ifdefs to keep using the improper type typeof($1) for non-FreeBSD=
?
Why these messy ifdefs in all places except the first one above?
> Modified: user/eri/pf45/head/contrib/pf/pfctl/pfctl_parser.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- user/eri/pf45/head/contrib/pf/pfctl/pfctl_parser.c=09Thu Feb 11 08:50=
:21 2010=09(r203776)
> +++ user/eri/pf45/head/contrib/pf/pfctl/pfctl_parser.c=09Thu Feb 11 09:55=
:45 2010=09(r203777)
> @@ -576,7 +576,7 @@ print_status(struct pf_status *s, int op
> =09=09for (i =3D 0; i < SCNT_MAX; i++) {
> =09=09=09printf(" %-25s %14lld ", pf_scounters[i],
> #ifdef __FreeBSD__
> -=09=09=09=09 (int64_t)s->scounters[i]);
> +=09=09=09=09 (long long)s->scounters[i]);
> #else
> =09=09=09=09 s->scounters[i]);
> #endif
The type s->scounters is easier to see than the type of $[13] in the
above. It is certainly not signed long long, at least in the public
version. It is u_int64_t. This type should be printed by converting
it to uintmax_t and printing it using format %ju. The above depends
on the following assumptions:
very old FreeBSD case (the public version):
The cast seems to have been almost correct. It was to unsigned long
long. That would work, but is ugly, provided the format is unbroken
too. The format %lld is only suitable for printing signed values,
but the value was unsigned long long. This gave undefined behaviour
if the value was > LONGLONG_MAX (which is possible though unlikely).
old version (in above):
Casting to int64_t gives undefined behaviour if sizeof(int64_t) !=3D
sizeof(long long). It happens that sizeof(int64_t) =3D=3D sizeof(long
long) for all machines curently supported by FreeBSD, so there is
no runtime error, but int64_t is carefully chosen so that the
compiler can detect this type mismatch anyway on 64-bit arches,
so that printf format errors don't need to be "fixed" for every
generation of word sizes. There is still a sign mismatch between
the variable type and the printf format. Now the cast is bug-for-
bug compatible with the format, so there is no undefined behaviour
for values > INT64_MAX. Such values are just corrupted by the
cast in an implementation-defined way, probably to negative values.
new version (in above):
Since LONG_LONG_MAX >=3D INT64_MAX, values up to and including
INT64_MAX can be printed correctly by converting them to long long
and using %lld format. There is still a sign mismatch between the
variable type and the printf format, as above. This could be fixed
by converting to unsigned long long and %llu format. Then there
would only be style bugs (use of the long long abomination).
non-FreeBSD version:
This remains plain broken. It depends on
sizeof(uint64_t) =3D=3D sizeof(long long), which is true today on some
machines, but will break someday (except I fear the people who
broke long by requiring it to be 32-bits forever for binary
compatibility will break long long similarly, requiring it to be
64 bits forever). It also depends on uint64_t being signed, which
it isn't, or on the value never exceeding INT64_MAX.
> Modified: user/eri/pf45/head/sys/contrib/pf/net/if_pfsync.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- user/eri/pf45/head/sys/contrib/pf/net/if_pfsync.c=09Thu Feb 11 08:50:=
21 2010=09(r203776)
> +++ user/eri/pf45/head/sys/contrib/pf/net/if_pfsync.c=09Thu Feb 11 09:55:=
45 2010=09(r203777)
> @@ -2805,7 +2805,7 @@ pfsync_q_ins(struct pf_state *st, int q)
> #if 1 || defined(PFSYNC_DEBUG)
> =09if (sc->sc_len < PFSYNC_MINPKT)
> #ifdef __FreeBSD__
> -=09=09panic("pfsync pkt len is too low %ld", sc->sc_len);
> +=09=09panic("pfsync pkt len is too low %zu", sc->sc_len);
> #else
> =09=09panic("pfsync pkt len is too low %d", sc->sc_len);
> #endif
>
This is correct iff sc_len has type size_t in FreeBSD and int in non-FreeBS=
D.
Its type shouldn't be so different.
Bruce
--0-619413657-1265980074=:89449--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100212231841.Q89449>