From owner-svn-src-head@FreeBSD.ORG Sat Jul 9 17:45:52 2011 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A635E106564A; Sat, 9 Jul 2011 17:45:52 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 22E4D8FC08; Sat, 9 Jul 2011 17:45:51 +0000 (UTC) Received: from c122-106-165-191.carlnfd1.nsw.optusnet.com.au (c122-106-165-191.carlnfd1.nsw.optusnet.com.au [122.106.165.191]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p69HjlX9019510 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 10 Jul 2011 03:45:48 +1000 Date: Sun, 10 Jul 2011 03:45:47 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Kevin Lo In-Reply-To: <201107090743.p697huB2086379@svn.freebsd.org> Message-ID: <20110710020439.U2667@besplex.bde.org> References: <201107090743.p697huB2086379@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed 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-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 09 Jul 2011 17:45:52 -0000 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