From owner-svn-src-all@FreeBSD.ORG Sun Jul 10 07:07:17 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4C211106564A; Sun, 10 Jul 2011 07:07:17 +0000 (UTC) (envelope-from kevlo@FreeBSD.org) Received: from ns.kevlo.org (kevlo.org [220.128.136.52]) by mx1.freebsd.org (Postfix) with ESMTP id 8AC7F8FC14; Sun, 10 Jul 2011 07:07:16 +0000 (UTC) Received: from [127.0.0.1] (kevlo@kevlo.org [220.128.136.52]) by ns.kevlo.org (8.14.3/8.14.3) with ESMTP id p6A77EqY031648; Sun, 10 Jul 2011 15:07:14 +0800 (CST) From: Kevin Lo To: Bruce Evans In-Reply-To: <20110710020439.U2667@besplex.bde.org> References: <201107090743.p697huB2086379@svn.freebsd.org> <20110710020439.U2667@besplex.bde.org> Content-Type: text/plain; charset="us-ascii" Date: Sun, 10 Jul 2011 15:07:14 +0800 Message-ID: <1310281634.2446.7.camel@srgsec> Mime-Version: 1.0 X-Mailer: Evolution 2.32.1 FreeBSD GNOME Team Port Content-Transfer-Encoding: 7bit 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 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Jul 2011 07:07:17 -0000 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