svn commit: r203777 - in user/eri/pf45/head: contrib/pf/pfctl sys/contrib/pf/net

[Bruce Evans]

On Thu, 11 Feb 2010, Ermal Luçi 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
> ==============================================================================
> --- user/eri/pf45/head/contrib/pf/pfctl/parse.y	Thu Feb 11 08:50:21 2010	(r203776)
> +++ user/eri/pf45/head/contrib/pf/pfctl/parse.y	Thu Feb 11 09:55:45 2010	(r203777)
> @@ -709,7 +709,7 @@ varstring	: numberstring varstring 		{
>
> numberstring	: NUMBER				{
> 			char	*s;
> -			if (asprintf(&s, "%lld", (int64_t)$1) == -1) {
> +			if (asprintf(&s, "%lld", (long long)$1) == -1) {

The long long type is always improper, as is the %lld format that goes
with it.

> @@ -2859,7 +2859,7 @@ host		: STRING			{
>
> 			/* ie. for 10/8 parsing */
> #ifdef __FreeBSD__
> -			if (asprintf(&buf, "%lld/%lld", (int64_t)$1, (int64_t)$3) == -1)
> +			if (asprintf(&buf, "%lld/%lld", (long long)$1, (long long)$3) == -1)
> #else
> 			if (asprintf(&buf, "%lld/%lld", $1, $3) == -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
> ==============================================================================
> --- user/eri/pf45/head/contrib/pf/pfctl/pfctl_parser.c	Thu Feb 11 08:50:21 2010	(r203776)
> +++ user/eri/pf45/head/contrib/pf/pfctl/pfctl_parser.c	Thu Feb 11 09:55:45 2010	(r203777)
> @@ -576,7 +576,7 @@ print_status(struct pf_status *s, int op
> 		for (i = 0; i < SCNT_MAX; i++) {
> 			printf("  %-25s %14lld ", pf_scounters[i],
> #ifdef __FreeBSD__
> -				    (int64_t)s->scounters[i]);
> +				    (long long)s->scounters[i]);
> #else
> 				    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) !=
     sizeof(long long).  It happens that sizeof(int64_t) == 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 >= 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) == 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
> ==============================================================================
> --- user/eri/pf45/head/sys/contrib/pf/net/if_pfsync.c	Thu Feb 11 08:50:21 2010	(r203776)
> +++ user/eri/pf45/head/sys/contrib/pf/net/if_pfsync.c	Thu Feb 11 09:55:45 2010	(r203777)
> @@ -2805,7 +2805,7 @@ pfsync_q_ins(struct pf_state *st, int q)
> #if 1 || defined(PFSYNC_DEBUG)
> 	if (sc->sc_len < PFSYNC_MINPKT)
> #ifdef __FreeBSD__
> -		panic("pfsync pkt len is too low %ld", sc->sc_len);
> +		panic("pfsync pkt len is too low %zu", sc->sc_len);
> #else
> 		panic("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-FreeBSD.
Its type shouldn't be so different.

Bruce