Date: Sun, 10 Jul 2011 15:07:14 +0800 From: Kevin Lo <kevlo@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r223877 - in head: include/rpc lib/libc/xdr Message-ID: <1310281634.2446.7.camel@srgsec> In-Reply-To: <20110710020439.U2667@besplex.bde.org> References: <201107090743.p697huB2086379@svn.freebsd.org> <20110710020439.U2667@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 2011-07-10 at 03:45 +1000, Bruce Evans wrote:
> 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?
Does it break style(9)?
> > 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.
Patches are welcome. Thanks!
> Bruce
Kevin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1310281634.2446.7.camel>