From owner-svn-src-head@FreeBSD.ORG Sun Jan 29 12:37:38 2012 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 BCDE51065672; Sun, 29 Jan 2012 12:37:38 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail01.syd.optusnet.com.au (mail01.syd.optusnet.com.au [211.29.132.182]) by mx1.freebsd.org (Postfix) with ESMTP id 039A18FC13; Sun, 29 Jan 2012 12:37:37 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail01.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q0TCbUPf031570 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 29 Jan 2012 23:37:33 +1100 Date: Sun, 29 Jan 2012 23:37:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Kostik Belousov In-Reply-To: <20120129062327.GK2726@deviant.kiev.zoral.com.ua> Message-ID: <20120129223232.F925@besplex.bde.org> References: <201201261159.q0QBxma2086162@svn.freebsd.org> <20120126233110.C960@besplex.bde.org> <20120126153641.GA68112@FreeBSD.org> <20120127194612.H1547@besplex.bde.org> <20120127091244.GZ2726@deviant.kiev.zoral.com.ua> <20120127194221.GA25723@zim.MIT.EDU> <20120128123748.GD2726@deviant.kiev.zoral.com.ua> <20120129001225.GA32220@zim.MIT.EDU> <20120129062327.GK2726@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Gleb Smirnoff , src-committers@freebsd.org, Bruce Evans Subject: Re: svn commit: r230583 - head/sys/kern 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: Sun, 29 Jan 2012 12:37:38 -0000 On Sun, 29 Jan 2012, Kostik Belousov wrote: > On Sat, Jan 28, 2012 at 07:12:25PM -0500, David Schultz wrote: >> On Sat, Jan 28, 2012, Kostik Belousov wrote: >>> On Fri, Jan 27, 2012 at 02:42:21PM -0500, David Schultz wrote: >>>> On Fri, Jan 27, 2012, Kostik Belousov wrote: >>>>> On Fri, Jan 27, 2012 at 07:50:30PM +1100, Bruce Evans wrote: >>>>>> On Thu, 26 Jan 2012, Gleb Smirnoff wrote: >>>>>> >>>>>>> On Thu, Jan 26, 2012 at 11:53:57PM +1100, Bruce Evans wrote: >>>>>>> B> > @@ -1552,6 +1552,12 @@ aio_aqueue(struct thread *td, struct aio >>>>>>> B> > return (error); >>>>>>> B> > } >>>>>>> B> > >>>>>>> B> > + /* XXX: aio_nbytes is later casted to signed types. */ >>>>>>> B> > + if ((int)aiocbe->uaiocb.aio_nbytes < 0) { >>>>>>> B> >>>>>>> B> This should avoid implementation-defined behaviour by checking if >>>>>>> B> >>>>>>> B> (uncast)aiocbe->uaiocb.aio_nbytes > INT_MAX. >>>>>> >>>>>>> Is the attached patch okay? >>>>>> >>>>>> Yes. It now matches the style used for read^Wsys_read() and friends. >>>>>> This used to have to fit the count in "int uio_resid". uio_resid now >>>>>> has type ssize_t, but for some reason the old INT_MAX limits remain. >>>>> >>>>> Well, I can revive the patch. I still think it is good to get rid of >>>>> the limit. >>>> >>>> The correct limit on the maximum size of a single read/write is >>>> SSIZE_MAX, but FreeBSD uses INT_MAX. It's not safe to raise the >>>> limit yet, though, because of bugs in several filesystems. For >>>> example, FFS copies uio_resid into a local variable of type int. >>>> I have some old patches that fix some of these issues for FFS and >>>> cd9660, but surely there are more places I didn't notice. >>>> >>> Absolutely agree. >>> >>> http://people.freebsd.org/~kib/misc/uio_resid.5.patch >> >> Nice. You found a lot more than I've got in my tree, and you even >> fixed the return values. There are at least a few more places to >> fix. For instance, cd9660 and the NFS client pass uio_resid or >> iov_len to min(), which operates on ints. (Incidentally, C11 >> generics ought to make it possible to write type-generic min() >> and max() functions.) So does gnu typeof(), and much more portably in practice (we're still waiting for a C99 compiler). > Thank you, http://people.freebsd.org/~kib/misc/uio_resid.6.patch > changed them to MIN(). Ugh, the existence of MIN() is API breakage (similarly for MAX()): - in FreeBSD-1 (and probably in Net/2), MIN() was min() in the kernel, where min() was an extern function with the same semantics as the current inline min(). This was quite broken, since MIN() is almost type-generic, while min() forces everything to u_int. min() was implemented in kern/subr_xxx.c. FreeBSD-1 (and maybe Net/2) also had imin/max(), lmin/max() and ulmin/max() there. - 4.4BSD completed removing MIN() in the kernel. It was left undefined. This was fragile, and probably still more broken than old code that used MIN(), since the conversions given by prototypes combined with no warnings for dangerous conversions tend to give even more sign extension and overflow bugs that the implicit conversions in MIN(). - MIN() remained intentionally left out in the kernel in FreeBSD-[2-4]. Some contribed code that didn't know the BSD API rolled its own MIN() and used that. There are no less than 38 home made definitions of MIN() in FreeBSD-4 to replace the one that is intentionally left out :-(. Some of these are ifdefed, but since the system doesn't have MIN(), they are always used. - the API was broken in FreeBSD-5 by removing the ifdef that left out MIN(). - even more code that doesn't know the old BSD API now uses MIN(). Now there are only 16 home made definitions of MIN(). Most of these are ifdefed. I have had (but not used) the following fairly type-generic and safe macros for min() since FreeBSD-~2.0: % Index: libkern.h % =================================================================== % RCS file: /home/ncvs/src/sys/sys/libkern.h,v % retrieving revision 1.45 % diff -u -2 -r1.45 libkern.h % --- libkern.h 7 Apr 2004 04:19:49 -0000 1.45 % +++ libkern.h 7 Apr 2004 11:31:02 -0000 % @@ -49,4 +51,74 @@ % #define hex2ascii(hex) (hex2ascii_data[hex]) % % +#if 0 % +#define __max(x, y) \ % +({ \ % + __typeof(x) __x = (x); \ % + __typeof(y) __y = (y); \ % + __x > __y ? __x : __y; \ % +}) % + % +#define __min(x, y) \ % +({ \ % + __typeof(x) __x = (x); \ % + __typeof(y) __y = (y); \ % + __x < __y ? __x : __y; \ % +}) % +#endif Normal use of gnu typeof() to write safe type-generic macros for things like this. This is almost verbatime from gcc.info. % + % +#define __max(x, y) \ % +( \ % + (sizeof(x) == 8 || sizeof(y) == 8) ? \ % + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \ % + _qmax((x), (y)) \ % + : \ % + _uqmax((x), (y)) \ % + : \ % + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \ % + _imax((x), (y)) \ % + : \ % + _max((x), (y)) \ % +) % + % +#define __min(x, y) \ % +( \ % + (sizeof(x) == 8 || sizeof(y) == 8) ? \ % + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \ % + _qmin((x), (y)) \ % + : \ % + _uqmin((x), (y)) \ % + : \ % + ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \ % + _imin((x), (y)) \ % + : \ % + _min((x), (y)) \ % +) Since I didn't want to use gccisms back in FreeBSD-~2.0 before use of gccisms became ubiquitous, I tried to use typeof() just to select the correct inline function like the programmer should. Errors were then detected by the above picking a different one than the one used causing the object file to change (if I was lucky, the object file didn't change due to just the extra complexity in the above). I committed a few fixes for type errors found by this. % + % +#ifdef MACRO_MAX % +static __inline int _imax(int a, int b) { return (a > b ? a : b); } % +static __inline int _imin(int a, int b) { return (a < b ? a : b); } % +static __inline long lmax(long a, long b) { return (a > b ? a : b); } % +static __inline long lmin(long a, long b) { return (a < b ? a : b); } % +static __inline u_int _max(u_int a, u_int b) { return (a > b ? a : b); } % +static __inline u_int _min(u_int a, u_int b) { return (a < b ? a : b); } % +static __inline quad_t _qmax(quad_t a, quad_t b) { return (a > b ? a : b); } % +static __inline quad_t _qmin(quad_t a, quad_t b) { return (a < b ? a : b); } % +static __inline u_long ulmax(u_long a, u_long b) { return (a > b ? a : b); } % +static __inline u_long ulmin(u_long a, u_long b) { return (a < b ? a : b); } % +static __inline u_quad_t _uqmax(u_quad_t a, u_quad_t b) { return (a > b ? a : b); } % +static __inline u_quad_t _uqmin(u_quad_t a, u_quad_t b) { return (a < b ? a : b); } Rename the inlines so that the top level API can use either them or the ones that do the selection. % + % +#define imax(a, b) __max((a), (b)) % +#define imin(a, b) __min((a), (b)) % +#define lmax(a, b) __max((a), (b)) % +#define lmin(a, b) __min((a), (b)) % +#define max(a, b) __max((a), (b)) % +#define min(a, b) __min((a), (b)) % +#define qmax(a, b) __max((a), (b)) % +#define qmin(a, b) __min((a), (b)) % +#define ulmax(a, b) __max((a), (b)) % +#define ulmin(a, b) __min((a), (b)) % +#else But normally, don't use any of the above additions. % +#ifdef __GNUC__ % static __inline int imax(int a, int b) { return (a > b ? a : b); } % static __inline int imin(int a, int b) { return (a < b ? a : b); } % @@ -63,4 +135,6 @@ % static __inline long labs(long a) { return (a < 0 ? -a : a); } % static __inline quad_t qabs(quad_t a) { return (a < 0 ? -a : a); } % +#endif % +#endif % % /* Prototypes for non-quad routines. */ What should happen is that everyone should use a type-generic safe macro named min() and not provide home made versions of it or MIN(), but the namespace has been horribly messed up by using min() for the u_int function. MIN() is not so broken, but since its name says that it is unsafe, careful users of it should be adding extra code in some cases to avoid multiple evaluations of its args, either for correctness or efficiency. I haven't noticed any problems from multiple evaluations for MIN(), but I noticed one for an mtx macro (most mtx APIs are macros, and are unsafe despite their name, and despite them copying a parameter to a local variable in a few places. Since they mostly return void, they can use the do-while hack to get a block with local variables, and don't require the extra unportability given by the gcc statement-expressions used in the above). Bruce