svn commit: r223877 - in head: include/rpc lib/libc/xdr
Bruce Evans
brde at optusnet.com.au
Sat Jul 9 17:45:52 UTC 2011
On Sat, 9 Jul 2011, Kevin Lo wrote:
> Log:
> - Add xdr_sizeof(3) to libc
> - Document xdr_sizeof(3); from NetBSD
>
> Discussed with: kib
Any reason to further break the style of every changed line of the header?
> Modified: head/include/rpc/xdr.h
> ==============================================================================
> --- head/include/rpc/xdr.h Fri Jul 8 20:41:12 2011 (r223876)
> +++ head/include/rpc/xdr.h Sat Jul 9 07:43:56 2011 (r223877)
> @@ -285,43 +285,46 @@ struct xdr_discrim {
> * These are the "generic" xdr routines.
> */
> __BEGIN_DECLS
> -extern bool_t xdr_void(void);
> -extern bool_t xdr_int(XDR *, int *);
> -extern bool_t xdr_u_int(XDR *, u_int *);
> -extern bool_t xdr_long(XDR *, long *);
> -extern bool_t xdr_u_long(XDR *, u_long *);
> -extern bool_t xdr_short(XDR *, short *);
> -extern bool_t xdr_u_short(XDR *, u_short *);
> -extern bool_t xdr_int16_t(XDR *, int16_t *);
> -extern bool_t xdr_u_int16_t(XDR *, u_int16_t *);
> -extern bool_t xdr_uint16_t(XDR *, u_int16_t *);
> -extern bool_t xdr_int32_t(XDR *, int32_t *);
> -extern bool_t xdr_u_int32_t(XDR *, u_int32_t *);
> -extern bool_t xdr_uint32_t(XDR *, u_int32_t *);
> -extern bool_t xdr_int64_t(XDR *, int64_t *);
> -extern bool_t xdr_u_int64_t(XDR *, u_int64_t *);
> -extern bool_t xdr_uint64_t(XDR *, u_int64_t *);
> -extern bool_t xdr_bool(XDR *, bool_t *);
> -extern bool_t xdr_enum(XDR *, enum_t *);
> -extern bool_t xdr_array(XDR *, char **, u_int *, u_int, u_int, xdrproc_t);
> -extern bool_t xdr_bytes(XDR *, char **, u_int *, u_int);
> -extern bool_t xdr_opaque(XDR *, char *, u_int);
> -extern bool_t xdr_string(XDR *, char **, u_int);
> -extern bool_t xdr_union(XDR *, enum_t *, char *, const struct xdr_discrim *, xdrproc_t);
> -extern bool_t xdr_char(XDR *, char *);
> -extern bool_t xdr_u_char(XDR *, u_char *);
> -extern bool_t xdr_vector(XDR *, char *, u_int, u_int, xdrproc_t);
> -extern bool_t xdr_float(XDR *, float *);
> -extern bool_t xdr_double(XDR *, double *);
> -extern bool_t xdr_quadruple(XDR *, long double *);
> -extern bool_t xdr_reference(XDR *, char **, u_int, xdrproc_t);
> -extern bool_t xdr_pointer(XDR *, char **, u_int, xdrproc_t);
> -extern bool_t xdr_wrapstring(XDR *, char **);
> -extern void xdr_free(xdrproc_t, void *);
> -extern bool_t xdr_hyper(XDR *, quad_t *);
> -extern bool_t xdr_u_hyper(XDR *, u_quad_t *);
> -extern bool_t xdr_longlong_t(XDR *, quad_t *);
> -extern bool_t xdr_u_longlong_t(XDR *, u_quad_t *);
Old brokenness: bogus externs (they have no effect, except for some
pre-K&R compilers, but even support for K&R was dropped long ago).
> +extern bool_t xdr_void(void);
> +extern bool_t xdr_int(XDR *, int *);
> +extern bool_t xdr_u_int(XDR *, u_int *);
> +extern bool_t xdr_long(XDR *, long *);
> +extern bool_t xdr_u_long(XDR *, u_long *);
> +extern bool_t xdr_short(XDR *, short *);
> +extern bool_t xdr_u_short(XDR *, u_short *);
> +extern bool_t xdr_int16_t(XDR *, int16_t *);
> +extern bool_t xdr_u_int16_t(XDR *, u_int16_t *);
> +extern bool_t xdr_uint16_t(XDR *, u_int16_t *);
> +extern bool_t xdr_int32_t(XDR *, int32_t *);
> +extern bool_t xdr_u_int32_t(XDR *, u_int32_t *);
> +extern bool_t xdr_uint32_t(XDR *, u_int32_t *);
> +extern bool_t xdr_int64_t(XDR *, int64_t *);
> +extern bool_t xdr_u_int64_t(XDR *, u_int64_t *);
> +extern bool_t xdr_uint64_t(XDR *, u_int64_t *);
> +extern bool_t xdr_bool(XDR *, bool_t *);
> +extern bool_t xdr_enum(XDR *, enum_t *);
> +extern bool_t xdr_array(XDR *, char **, u_int *, u_int, u_int,
> + xdrproc_t);
> +extern bool_t xdr_bytes(XDR *, char **, u_int *, u_int);
> +extern bool_t xdr_opaque(XDR *, char *, u_int);
> +extern bool_t xdr_string(XDR *, char **, u_int);
> +extern bool_t xdr_union(XDR *, enum_t *, char *,
> + const struct xdr_discrim *, xdrproc_t);
> +extern bool_t xdr_char(XDR *, char *);
> +extern bool_t xdr_u_char(XDR *, u_char *);
> +extern bool_t xdr_vector(XDR *, char *, u_int, u_int, xdrproc_t);
> +extern bool_t xdr_float(XDR *, float *);
> +extern bool_t xdr_double(XDR *, double *);
> +extern bool_t xdr_quadruple(XDR *, long double *);
> +extern bool_t xdr_reference(XDR *, char **, u_int, xdrproc_t);
> +extern bool_t xdr_pointer(XDR *, char **, u_int, xdrproc_t);
> +extern bool_t xdr_wrapstring(XDR *, char **);
> +extern void xdr_free(xdrproc_t, void *);
> +extern bool_t xdr_hyper(XDR *, quad_t *);
> +extern bool_t xdr_u_hyper(XDR *, u_quad_t *);
> +extern bool_t xdr_longlong_t(XDR *, quad_t *);
> +extern bool_t xdr_u_longlong_t(XDR *, u_quad_t *);
New style bugs in old code: excessive indentation (to match the excessive
indentation in 1 line of new code).
> +extern unsigned long xdr_sizeof(xdrproc_t, void *);
New style bugs in new code: verboseness and associated style
inconsistencies. 'unsigned foo' is spelled u_foo in KNF to avoid
verboseness, and that style is used everywhere else in this file, even
more so than usual since even the function names are spelled with
u_foo. If u_foo were spelled normally, the indentation would not be
excessive. This is the main reason for using u_foo.
I think it also has an non-style bug -- a type mismatch. It returns
u_long instead of the usual bool_t since it returns sizes of ojects. But
size_t doesn't always have type unsigned long. It has type size_t, which
is u_int on all 32-bit arches. This bug is old. xdr_size_t() is full
of type errors -- see below.
> ...
> Modified: head/lib/libc/xdr/xdr.3
> ==============================================================================
> --- head/lib/libc/xdr/xdr.3 Fri Jul 8 20:41:12 2011 (r223876)
> +++ head/lib/libc/xdr/xdr.3 Sat Jul 9 07:43:56 2011 (r223877)
> @@ -31,6 +31,7 @@
> .Nm xdr_reference ,
> .Nm xdr_setpos ,
> .Nm xdr_short ,
> +.Nm xdr_sizeof,
> .Nm xdrstdio_create ,
> .Nm xdr_string ,
> .Nm xdr_u_char ,
> @@ -561,6 +562,18 @@ A filter primitive that translates betwe
> integers and their external representations.
> This routine returns one if it succeeds, zero otherwise.
> .Pp
> +.It Xo
> +.Ft unsigned long
Another `unsigned long'. The man page was already less consistent
than the header in using u_foo. It uses `unsigned foo' for xdr_u_char(),
xdr_u_short() and xdr_u_long, plain unsigned for xdr_u_int(), and
uquad_t (sic) for xdr_ulonglong_t() (sic), so that the arg type doesn't
match the function name literally for these functions. OTOH, it uses
u_foo consistently for more specialized args (like counts and sizes)
whose type isn't determined by the type of the data being translated.
> +.Xc
> +.It Xo
> +.Fn xdr_sizeof "xdrproc_t func" "void *data"
> +.Xc
Using .Xo/.Xc is probably another style bug, but this man page does it
consistently.
> +.Pp
> +This routine returns the amount of memory required to encode
> +.Fa data
> +using filter
> +.Fa func .
> +.Pp
> .It Li "#ifdef _STDIO_H_"
> .It Li "/* XDR using stdio library */"
> .It Xo
>
> Modified: head/lib/libc/xdr/xdr_sizeof.c
> ==============================================================================
> --- head/lib/libc/xdr/xdr_sizeof.c Fri Jul 8 20:41:12 2011 (r223876)
> +++ head/lib/libc/xdr/xdr_sizeof.c Sat Jul 9 07:43:56 2011 (r223877)
> @@ -94,7 +94,7 @@ x_inline(xdrs, len)
> if (xdrs->x_op != XDR_ENCODE) {
> return (NULL);
> }
> - if (len < (u_int)xdrs->x_base) {
> + if (len < (u_int)(uintptr_t)xdrs->x_base) {
> /* x_private was already allocated */
> xdrs->x_handy += len;
> return ((int32_t *) xdrs->x_private);
Shouldn't the u_int be size_t? u_int doesn't match size_t any better than
u_long does.
I now see that the return type of unsigned long is an old mistake too.
Dusty decks like rpc have zillions of type errors. The next one
(visible in the above) is int32_t instead of any of size_t, the return
type or u_int. Then xdr_sizeof() uses the caddr_t mistake. Then at
the end, xdr_sizeof() bogusly casts the value to be returned to unsigned
(spelled as plain unsigned) instead of to any of size_t, the return
type, u_int or int32_t.
> @@ -106,7 +106,7 @@ x_inline(xdrs, len)
> xdrs->x_base = 0;
> return (NULL);
> }
> - xdrs->x_base = (caddr_t) len;
> + xdrs->x_base = (caddr_t)(uintptr_t)len;
> xdrs->x_handy += len;
> return ((int32_t *) xdrs->x_private);
> }
>
xdr_inline() has similar type errors, but its return type is int32_t
and it casts to that fairly consistently.
I noticed the following other old bugs in the man page:
- all the functions that are declared as returning bool_t are documented
as returning int. bool_t is only documented to be used once (for the
xdr_bool() pointer type. I prefer to use plain int.
- size_t isn't used for any API. u_int and not u_long is used for all
the size args that should really have type size_t.
- the phrase "translates between [unsigned] ANSI C long long integers and
their external representations" is used ad nauseum, but ANSI C doesn't
exist. Informally, "ANSI C" means the pre-ISO-C90, and neither it
not C90 have the long long mistake, so long long integers are further
from being "ANSI C" than all other integers. For the actuall-ANSI-C
integers, the duplicated phrase says "C [unsigned]" instead of
"[unsigned] ANSI C" (note that this is also missing the bug of
misplacing "[unsigned]"). The phrase for xdr_bool() says that
booleans (bool_t's) are C integers, so bool_t must be int even in
the one place that it is documented, except it is not clear that the
"integer" here must be int.
- xdr_longlong_t() and xdr_ulonglong_t() look like they support the
[unsigned] long long mistake, and are documented to translate between
[unsigned] ANSI C long long integers and their external representations,
but their arg type is [u_]quad_t, so they actually only support BSD
[unsigned] quad integers. Support for other C99 types is similarly
absent (but present in practice in the same way as for long long while
quad_t remains the same as long long, int64_t and intmax_t, and similarly
for other intN_t).
- xdr_longlong_t() and xdr_ulonglong_t() have a bogus _t in their name.
Another man page, rpc.3, says that the xdr's x_inline function pointer
is for a function that returns `long *'. This is inconsistent with
the int32_t returns for the xdr x_inline function remarked on above.
It is also inconsistent with the actual declaration of the x_inline
function pointer in xdr.h. That uses int32_t too. It was changed to
use int32_t instead of long in 1996.
Bruce
More information about the svn-src-all
mailing list